diff --git a/.cursor/rules/README.md b/.cursor/rules/README.md new file mode 100644 index 00000000..20f44b51 --- /dev/null +++ b/.cursor/rules/README.md @@ -0,0 +1,16 @@ +# Cursor Rules for QDrill Project + +This directory contains project-specific rules for AI assistants working on the QDrill codebase. + +## Rules Files + +- `development.mdc` - Core development commands and workflows + +## Key Points to Remember + +1. **ALWAYS use `vercel dev`** for development server (NOT pnpm run dev) +2. Use `pnpm` for package management (NOT npm) +3. Use `vercel build` for production builds +4. Use `psql "$NEON_DB_URL"` for database queries + +These rules help ensure consistent development practices across all AI assistant sessions. diff --git a/.cursor/rules/development.mdc b/.cursor/rules/development.mdc new file mode 100644 index 00000000..4fd15f99 --- /dev/null +++ b/.cursor/rules/development.mdc @@ -0,0 +1,33 @@ +--- +description: +globs: +alwaysApply: true +--- +# Development Rules for QDrill Project + +## IMPORTANT: Development Server +**ALWAYS use `vercel dev` to run the development server** +- DO NOT use `pnpm run dev` +- DO NOT use `npm run dev` +- The correct command is: `vercel dev` + +## Package Management +This project uses pnpm. Always use pnpm commands: +- `pnpm install` (not npm install) +- `pnpm add ` (not npm install ) +- `pnpm remove ` (not npm uninstall) + +## Build Commands +- For development server: `vercel dev` +- For production build verification: `vercel build` +- DO NOT use `pnpm run build` for verification + +## Database Access +When examining data, use: `psql "$NEON_DB_URL"` + +## Summary of Key Commands +- Start dev server: `vercel dev` +- Install dependencies: `pnpm install` +- Add package: `pnpm add ` +- Build for production: `vercel build` +- Check database: `psql "$NEON_DB_URL"` diff --git a/.cursorignore b/.cursorignore new file mode 100644 index 00000000..6f9f00ff --- /dev/null +++ b/.cursorignore @@ -0,0 +1 @@ +# Add directories or file patterns to ignore during indexing (e.g. foo/ or *.csv) diff --git a/.eslintrc.cjs b/.eslintrc.cjs index 29c1d31b..035dca17 100644 --- a/.eslintrc.cjs +++ b/.eslintrc.cjs @@ -1,14 +1,45 @@ module.exports = { root: true, - extends: ['eslint:recommended', 'plugin:svelte/recommended', 'prettier'], + extends: [ + 'eslint:recommended', + 'plugin:svelte/recommended', + 'plugin:cypress/recommended', + 'prettier' + ], + plugins: ['vitest-globals'], parserOptions: { + parser: '@typescript-eslint/parser', sourceType: 'module', - ecmaVersion: 2020, + ecmaVersion: 2022, extraFileExtensions: ['.svelte'] }, env: { browser: true, es2017: true, - node: true - } + node: true, + 'cypress/globals': true + }, + overrides: [ + { + files: ['*.js'], + parser: '@typescript-eslint/parser', + parserOptions: { + ecmaVersion: 2022, + sourceType: 'module' + } + }, + { + files: ['**/__tests__/**/*.js', '**/__mocks__/**/*.js'], + extends: ['plugin:vitest-globals/recommended'] + }, + { + files: ['.svelte'], + parser: 'svelte-eslint-parser', + parserOptions: { + parser: '@typescript-eslint/parser', + ecmaVersion: 2022, + sourceType: 'module' + } + } + ] }; diff --git a/.gitignore b/.gitignore index 8f6c617e..dabbdb7b 100644 --- a/.gitignore +++ b/.gitignore @@ -3,6 +3,9 @@ node_modules /build /.svelte-kit /package +venv +app/__pycache__ +*.pyc .env .env.* !.env.example @@ -10,3 +13,13 @@ node_modules .output vite.config.js.timestamp-* vite.config.ts.timestamp-* +.env*.local +/coverage +.nyc_output +tests/__pycache__ +tests/_pycache +cypress/downloads +cypress/screenshots + +# Sentry Config File +.env.sentry-build-plugin diff --git a/.prettierrc b/.prettierrc index a77fddea..95730232 100644 --- a/.prettierrc +++ b/.prettierrc @@ -4,6 +4,5 @@ "trailingComma": "none", "printWidth": 100, "plugins": ["prettier-plugin-svelte"], - "pluginSearchDirs": ["."], "overrides": [{ "files": "*.svelte", "options": { "parser": "svelte" } }] } diff --git a/.vscode/settings.json b/.vscode/settings.json new file mode 100644 index 00000000..31aca745 --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1,13 @@ +{ + "svelte.plugin.svelte.diagnostics.a11y": false, + "svelte.plugin.svelte.compilerWarnings": { + "a11y-click-events-have-key-events": "ignore", + "a11y-missing-attribute": "ignore", + "a11y-missing-content": "ignore", + "a11y-invalid-attribute": "ignore", + "a11y-label-has-associated-control": "ignore", + "a11y-no-static-element-interactions": "ignore", + "a11y-img-redundant-alt": "ignore", + "a11y-no-noninteractive-element-interactions": "ignore" + } +} diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 00000000..6e5d4c7b --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,220 @@ +# QDrill Project Guide + +## Project Overview + +QDrill is a web-based application designed as a sports drill bank and practice planning tool for a niche sport. It allows users to create, manage, and share drills, as well as plan practices with timeline-based organization. + +## Technology Stack + +- **Frontend**: SvelteKit +- **Backend**: SvelteKit (API routes) +- **Database**: PostgreSQL (via Vercel Postgres) +- **Styling**: Tailwind CSS +- **Authentication**: Auth.js (Google OAuth) +- **Deployment**: Vercel +- **Diagramming**: Excalidraw +- **Testing**: Playwright, Cypress + +## Core Features + +### Drill Management + +- Create, edit, view, and search drills +- Tag drills with skill level, positions, duration +- Upload diagrams/images for drills +- Upvote/downvote and comment on drills +- Create variations of existing drills + +### Practice Plan Creation + +- Wizard-based practice plan creation +- Section organization with parallel timelines +- Drag-and-drop editing interface +- Duration tracking and management +- Timeline visualization +- Share and duplicate practice plans + +### User System + +- Google OAuth authentication +- User profiles +- Permission-based access control +- Personal drill/plan management + +## Development Commands + +- `vercel dev` - Start development server +- `pnpm run check` - Check TypeScript + SvelteKit sync + +## Package Management + +- `pnpm install` - Install dependencies +- `pnpm add ` - Add a package + +## Testing + +- `pnpm run test` - Run Playwright tests +- `pnpm test -- tests/test.js` - Run a specific test +- `pnpm run test:unit` - Run Vitest unit tests in watch mode +- `pnpm run test:unit:run` - Run Vitest unit tests once +- `pnpm run test:unit:coverage` - Run Vitest unit tests with coverage +- `pnpm run test:unit --run ` - Run specific Vitest tests once (e.g., `pnpm run test:unit --run src/lib/stores/__tests__/dragManager.test.js`) + +## Code Quality + +- `pnpm run lint` - Run linting checks +- `pnpm run format` - Fix formatting issues + +## Deployment + +- Automatic deployment from GitHub main branch to Vercel + +## Architecture + +### Frontend Components + +- Svelte components organized by feature area +- Modular design with reusable components +- Tailwind CSS for styling with custom components +- Interactive drag-and-drop interface for practice planning + +### State Management + +- Extensive use of Svelte stores +- Separate stores for different domain concerns +- Custom store implementation with methods +- History tracking with undo/redo support + +### API Design + +- RESTful API endpoints +- SvelteKit server endpoints (+server.js) +- Standardized response formats +- Parameterized database queries + +### Database + +- PostgreSQL with connection pooling +- Transaction support for complex operations +- Normalized schema design + +#### Database Connection + +- **Connection String**: Stored in ~/.zshrc as NEON_DB_URL environment variable +- **Connection Command**: `psql "$NEON_DB_URL"` +- **Project ID**: morning-mountain-82887088 +- **Database Name**: verceldb +- **Tables**: drills, practice_plans, practice_plan_sections, practice_plan_drills, users, comments, votes, etc. +- **Query Example**: `psql "$NEON_DB_URL" -c "SELECT COUNT(*) FROM drills;"` + +## Key Systems + +### Authentication + +- Auth.js (formerly NextAuth) integration +- Google OAuth provider +- Session-based authentication +- Authorization middleware + +### Drag and Drop + +- Complex drag-and-drop system for practice plan editing +- State management via Svelte stores +- Multiple drop targets and interactions +- Timeline-based organization +- Visual feedback during drag operations + +### Data Filtering + +- Client-side filtering for drills +- Multi-criteria filtering +- Performance optimization for large datasets + +## Code Style Guidelines + +- **AI-Readability**: Add clear comments to make code easily understood by future AI systems +- **Comments**: Include purpose explanations, input/output expectations, and logic clarifications +- **Imports**: Group imports by source (svelte, lib, components) +- **Components**: Use Svelte components with script/markup/style structure +- **Stores**: Use reactive declarations with $ prefix for store values +- **Error Handling**: Use try/catch with specific error messages +- **API Endpoints**: Return standardized JSON responses with proper status codes +- **Database**: Use parameterized queries to prevent SQL injection +- **Naming**: Use descriptive camelCase for variables/functions, PascalCase for components + +## Areas for Improvement + +### 1. Drag and Drop System Consolidation + +**Impact: High** + +- Currently has two parallel drag-and-drop systems +- Consolidate into a single system with consistent interface +- Implement proper state machine for drag operations +- Reduce code complexity and maintenance burden + +### 2. API Data Fetching Abstraction + +**Impact: High** + +- Direct fetch calls scattered throughout components +- Create unified API client with standard methods +- Implement consistent error handling and retry logic +- Add caching and performance optimizations + +### 3. Test Coverage Expansion + +**Impact: High** + +- Minimal testing despite complex UI interactions +- Add unit tests for store logic (especially drag-and-drop) +- Implement integration tests for key user flows +- Set up CI pipeline with automated testing + +### 4. Store Logic Separation + +**Impact: Medium** + +- Store files mix different concerns (data, filtering, etc.) +- Separate into dedicated modules with single responsibilities +- Move complex logic into utility functions +- Improve maintainability and testability + +### 5. Performance Optimization + +**Impact: Medium** + +- Filtering/sorting recalculates on every store update +- Implement memoization for expensive calculations +- Use web workers for heavy operations +- Optimize filter chains and add virtualization + +## Documentation Workflow + +- After completing any significant task, ALWAYS follow this documentation workflow: + +1. First examine `/docs/index.md` to understand the documentation structure +2. Then navigate to the appropriate subdirectory based on the nature of your changes: + - `/docs/architecture/` for architectural changes or patterns + - `/docs/implementation/` for implementation details and technical references +3. Update existing documentation files or create new ones as needed +4. Update index files to reference any new documentation + +## Documentation Requirements + +- Create/update documentation when modifying .js/.svelte files +- Document component descriptions, usage instructions, and relationships +- Maintain documentation consistency for directory structure +- Consider component interdependencies when making changes +- Follow best practices for Svelte documentation +- Add implementation notes to `/docs/implementation/` for technical patterns +- **README Updates**: Always update the README.md file after completing substantial code edits to reflect the latest changes, features, and usage instructions + +## Version Control Guidelines + +- **Commit Message Standards**: Write clear, descriptive commit messages explaining what changes were made and why +- **Atomic Commits**: Keep commits focused on a single logical change +- **Pull Request Format**: Include clear descriptions of changes, impact, and testing performed +- **Code Reviews**: Request code reviews for substantial changes +- **No Automatic Commits**: Never commit changes without explicitly being asked to do so +- **Testing Before Commit**: Always run relevant tests before creating a commit diff --git a/East April TC Practice.docx.md b/East April TC Practice.docx.md new file mode 100644 index 00000000..a634d3f4 --- /dev/null +++ b/East April TC Practice.docx.md @@ -0,0 +1,154 @@ +**East April TC Practice** +April 26, 2025 // 8:30am-12pm +Ben Franklin Superdome (191 Knoxdale Rd, Ottawa ON, K2G 5K6) +[Topscore](https://quidditchcanada.usetopscore.com/en_ca/e/team-canada-eastern-practice-april) +[Rides & Accommodations](https://docs.google.com/spreadsheets/d/171X4W3C-ZE6gGnmhfqgBvcUAaHfeUOcU8zf-Bvf1_ug/edit?gid=0#gid=0) + +**(8:30) Outdoor Warm up** + +- Cleats already on by 8:30am so _arrive earlier_\! +- @ Fields by parking lot +- The usual + +**(9:00) Position Specific Warm ups** + +- Chasers + - Five star + - Long-short-long +- Beaters + - ? +- Volunteers + - Set up pitch + +**(9:15) Position Specific Drills** + +- Chasers + - (15 min) tackling + - 2v2, game-like scenario + - Darren to give some pointers + - (15 min) passing around hoops + - focused on triangle positioning and hard, accurate throws +- Beaters + - (15 min) making beats on ball carrier around free beater + - (15 min) Boston beater drill +- Seekers + - See seeker-specific plan @ end of document + +**(9:45) TC Strategic One Runs** + +**(10 min/9:45) General Strategic Intro** + +- Introduce some basic strategic concepts so that we can build on them in our next practices +- Looking to build chemistry and try new things (our offense needs to improve from previously) +- Defense has two bludgers in all scenarios +- Focused on “standard” defenses (no aggressive positioning or presses) + + - Use hoop and 2-2 as defensive formations + +- **General Offensive Notes** + - Chasers + - Basic intro to box and kite formations + - **Try to be coordinated and deliberate to start offense** + - Have an initial plan to shift defense + - Eg: pick on keeper from right + - Eg: get the ball deep and try to draw defense out + - Eg: fake handoff then pass back to center of kite + - **Use resets and consider reset passes as a success** + - **Take space before passing** + - Forces defensive shifts and commitment + - Beaters + - Goal is always to score, never recovery + - 80% free beater helps chasers attack, loaded beater protects ball carrier + - 15% loaded beater helps with very low risk + - Tap beats, fakes, protecting deep chaser + - Throws back when pressured + - 5% using our bludger to help offense + - Throw bludger at opposing beater from behind their hoops + - 0% throw our bludger at defensive beaters from in front + +**(10 min/9:55) Hoop Defense** + +- Outline hoop defense + - Only with 2 bludgers + - Keeper on keeper line + - Beaters in between hoops + - Beaters are initial pressure when ball is rotated behind hoops + - Do we rotate chaser out to ball as well? Or only beater? + - Does keeper stay near keeper line or rotate to hoops? +- Run free form offense + +**(30 min/10:05) Attacking VS Hoop** + +- Concept 1 (patience) + - Box + - Hard passes around hoops + - Pass deep, back to top, across to support, deep, back to support + - Everyone begins taking space when they have the ball then passes to a safe option +- Concept 2 (Ball-carrier initiated offense) + - Box + - Loaded beater protects a deep chaser who is passed the ball + - Support chaser looks to cut or pick at near side hoop + - take space from behind and attack +- Concept 3 (wing initiated offense) + - Kite + - Coordinate which sides beaters attack and attack same side + - A support chaser picks the keeper + - Deep chaser picks side hoop or is open for cut + +**(10 min/10:35) 2-2 Defense** + +- Outline 2-2 defense + - Can cheat away from hoops when ball is not in a scorable position + - Beater closes out to deep pass, chaser follows for pick up + - Defense “flips” to face wherever the ball is +- Run free form offense + +**(30 min/10:45) Attacking vs 2-2** + +- Concept 1 (patience) + - Box + - Hard passes around hoops + - Pass deep, back to top, across to support, deep, back to support + - Everyone begins taking space when they have the ball then passes to a safe option + - Against a 2-2 we need the support chasers and beater to actively ensure the reset passes are easily available +- Concept 2 + - Kite + - “pop” play + - Loaded beater taps the non-keeper point defender + - Ball carrier takes space and passes to now uncovered support chaser + - One beater recovers bludger, other harasses near side beater (goal is to block the beat) + - Drive with goal to dunk or pass to behind the hoops cutter + - This play also has a box version, can mix it up +- Concept 3 + - Kite + - Decide side for free beater to harass + - Fake drive and handoff on opposite side of beater + - Drive toward remaining point defender + - Support chaser either picks point defender or cuts to hoop + - Wing chaser either picks hoop defender or provides a safe passing option behind hoops + +**(15 min/11:15) Buffer** + +**(11:30) Scrimmages** + +- 3 8-minute scrimmages +- 2 quaffle period scenarios +- 1 SOP in range scenario + +///////// + +**(9:15 \- 11:30) Seeker Specific Drills** _(Christos & Ugo, Sav \- until one runs)_ + +- Catching drill with progression + - Claw drill + - leg load and dive + - full dive +- 1v1 with snitch + - primary move + - counter + - off-hand + - persistent pursuit + - fast approach +- 2v1 with snitch + - jockeying for position + - defensive box outs diff --git a/PR_111_NAVIGATION_REVIEW.md b/PR_111_NAVIGATION_REVIEW.md new file mode 100644 index 00000000..e38253b7 --- /dev/null +++ b/PR_111_NAVIGATION_REVIEW.md @@ -0,0 +1,38 @@ +# PR #111 Navigation Accessibility Review + +## Overview +Reviewed PR #111 (pqsyol-codex/update-ticket-for-ux-improvements-navigation-accessibility) which implements navigation accessibility improvements. + +## Testing Results + +### ✅ Successfully Implemented +1. **Click-based dropdowns** - Hover behavior removed, dropdowns now open on click +2. **Keyboard navigation** - Escape key closes dropdowns, arrow keys work for menu navigation +3. **ARIA attributes** - aria-expanded, aria-haspopup, aria-controls properly implemented +4. **Skip to main content** - Link present and functional with proper CSS +5. **Focus management** - Click outside closes dropdowns +6. **Mobile menu** - Hamburger menu with proper toggle functionality +7. **Global focus styles** - 2px solid blue outline with offset +8. **Color contrast improvements** - Darker grays for better readability +9. **Reduced motion support** - CSS media query implemented + +### ✅ All Features Working +After thorough testing, ALL accessibility features are working correctly, including active section highlighting. + +## Verification Details + +### Active Section Highlighting +**Status**: Working correctly +- The `isActiveSection()` function properly applies highlighting +- When on /drills page, the Drills button correctly shows: + - Light blue background (`bg-blue-50`) + - Blue text color (though subtle due to existing gray classes) +- JavaScript inspection confirmed classes are applied: `text-blue-600 bg-blue-50` + +## Recommendation +PR #111 is ready to merge. All accessibility features are implemented and working correctly. + +## Next Steps +1. Merge PR #111 +2. Proceed with PR #126 for additional accessibility improvements +3. Consider making the active section highlighting more prominent (the blue text is subtle due to CSS specificity) \ No newline at end of file diff --git a/PR_114_ERROR_HANDLING_REVIEW.md b/PR_114_ERROR_HANDLING_REVIEW.md new file mode 100644 index 00000000..61812e31 --- /dev/null +++ b/PR_114_ERROR_HANDLING_REVIEW.md @@ -0,0 +1,84 @@ +# PR #114 Error Handling UX - Review + +## Overview +Branch: `nhckfu-codex/update-ux-improvements-error-handling.md` +Status: ✅ Ready to merge with minor test failures + +## Implementation Review + +### ✅ Successfully Implemented + +1. **Custom Error Page Component** (`src/routes/+error.svelte`) + - Beautiful custom error pages for 404, 403, 500, and other errors + - Context-appropriate actions (Go Home, Try Again, Search, etc.) + - Search functionality for 404 pages + - Proper error logging for monitoring + +2. **Error Boundary Component** (`src/lib/components/ErrorBoundary.svelte`) + - Reusable error boundary with retry functionality + - Wraps main layout content in `+layout.svelte` + - Graceful error display with helpful messaging + +3. **Enhanced API Error Handling** (`src/lib/utils/errorHandling.js`) + - `APIError` class for structured errors + - `handleAPIError` function with context-aware messages + - Network error detection + - Status code specific messaging + +4. **apiFetch Enhanced** (`src/lib/utils/apiFetch.js`) + - Now throws `APIError` instances + - Better network error handling + - Improved error messages and logging + +5. **Whiteboard Error Handling** (`src/routes/whiteboard/+page.svelte`) + - Specific error UI for Excalidraw loading failures + - Retry functionality + - Troubleshooting tips (refresh, check connection, try different browser) + +6. **JavaScript Disabled Fallback** (`src/app.html`) + - Noscript message for users without JavaScript + +### 🔍 Testing Results + +The implementation is working well but has some existing test failures: +- Unit tests are failing due to unrelated issues with drill ID API tests (Zod validation) +- Type checking has some warnings but no breaking errors + +### 💡 Strengths + +1. **User-Friendly Error Messages**: Clear, actionable error messages instead of technical jargon +2. **Consistent Design**: Error pages maintain site branding and navigation +3. **Recovery Options**: Multiple pathways to recover from errors +4. **Accessibility**: Error pages are properly structured and accessible +5. **Monitoring Ready**: Error logging is in place for future monitoring integration + +### 🎯 Acceptance Criteria Met + +- ✅ Custom 404 page displays with helpful navigation options +- ✅ Server errors show branded error page with recovery suggestions +- ✅ API errors provide clear, actionable error messages +- ✅ Whiteboard loading errors have specific troubleshooting guidance +- ✅ All error pages maintain site navigation and branding +- ✅ Error pages are accessible and mobile-friendly +- ✅ Network errors are handled gracefully with retry options +- ✅ JavaScript disabled fallback message + +### 📝 Minor Issues + +1. **Test Failures**: Existing unit test failures unrelated to error handling +2. **Type Warnings**: Some TypeScript warnings in button components + +### 🚀 Recommendation + +**APPROVE AND MERGE** - The error handling implementation is comprehensive and production-ready. The test failures are pre-existing and not related to these changes. + +## Testing the Implementation + +To test the error handling: + +1. **404 Error**: Navigate to `/nonexistent-page` +2. **API Error**: Disconnect network and try to load drills +3. **Whiteboard Error**: Can be tested by blocking Excalidraw resources +4. **JavaScript Disabled**: Disable JavaScript and load any page + +All error scenarios provide helpful, branded experiences that guide users back to productive paths. \ No newline at end of file diff --git a/PR_116_CLEAR_RESET_FILTERS_REVIEW.md b/PR_116_CLEAR_RESET_FILTERS_REVIEW.md new file mode 100644 index 00000000..eaf06aa6 --- /dev/null +++ b/PR_116_CLEAR_RESET_FILTERS_REVIEW.md @@ -0,0 +1,81 @@ +# PR #116 Clear/Reset Filters - Review + +## Overview +Branch: `kvf6vx-codex/update-ticket-for-ux-improvements-clear-reset-filters` +Status: ✅ Ready to merge + +## Implementation Review + +### ✅ Successfully Implemented + +1. **Reset Filters Button Visibility** + - Fixed the issue where Reset Filters button only showed for drills + - Now displays for both `filterType === 'drills'` and `filterType === 'practice-plans'` + - Uses reactive checks: `hasActiveDrillFilters` and `hasActivePracticePlanFilters` + +2. **Practice Plan Active Filters Check** + ```javascript + $: hasActivePracticePlanFilters = + Object.keys($selectedPhaseOfSeason).length > 0 || + Object.keys($selectedPracticeGoals).length > 0 || + $selectedEstimatedParticipantsMin !== 1 || + $selectedEstimatedParticipantsMax !== 100 || + selectedDrills.length > 0; + ``` + +3. **Unified Reset Button** + ```svelte + {#if (filterType === 'drills' && hasActiveDrillFilters) || (filterType === 'practice-plans' && hasActivePracticePlanFilters)} + + {/if} + ``` + +4. **Practice Plans Page Integration** + - `FilterPanel` component correctly receives `filterType="practice-plans"` prop + - Filter changes trigger URL updates via `handleFilterChange` event + +### 🔍 Testing Results + +The implementation correctly addresses the issue where practice plan filters had no way to be reset. The button now appears when practice plan filters are active. + +### 💡 Strengths + +1. **Consistent UX**: Same reset button styling and behavior for both drills and practice plans +2. **Smart Visibility**: Button only shows when filters are actually applied +3. **Proper State Management**: Correctly tracks all practice plan filter states +4. **URL Integration**: Clearing filters properly updates the URL + +### 🎯 Acceptance Criteria Met + +- ✅ Clear/Reset filters button appears on the practice plans page when filters are active +- ✅ Button is easily discoverable and clearly labeled +- ✅ Clicking clears all applied practice plan filters +- ✅ URL is updated to remove practice plan filter parameters +- ✅ Existing reset functionality for drills remains unaffected +- ✅ Accessibility maintained with proper button semantics + +### 📝 Code Quality + +- Clean implementation that reuses existing `resetFilters` function +- No duplication of reset logic +- Proper reactive statements for filter state tracking +- Consistent with existing codebase patterns + +### 🚀 Recommendation + +**APPROVE AND MERGE** - This PR successfully implements the clear/reset filters functionality for practice plans without breaking existing drill filters. The implementation is clean and follows established patterns. + +## Testing the Implementation + +To test: +1. Navigate to `/practice-plans` +2. Apply various filters (phase of season, practice goals, participants, etc.) +3. Verify the "Reset Filters" button appears +4. Click the button and confirm all filters are cleared +5. Check that the URL parameters are removed +6. Test that drill page reset functionality still works independently \ No newline at end of file diff --git a/PR_133_vs_136_COMPARISON.md b/PR_133_vs_136_COMPARISON.md new file mode 100644 index 00000000..93c3d5dc --- /dev/null +++ b/PR_133_vs_136_COMPARISON.md @@ -0,0 +1,207 @@ +# PR #133 vs PR #136 State Management Refactoring Comparison + +## Overview + +Both PRs attempt to refactor state management for the practice plan drag-and-drop functionality, but they take fundamentally different approaches: + +- **PR #133**: Refactors `dragManager.js` to use `moveItem`, `moveSection` helper functions from `sectionsStore` +- **PR #136**: Refactors `sectionsStore.js` with new helper functions and updates `dragManager.js` to use `updateSections` + +## Key Differences + +### 1. Architectural Approach + +**PR #133 (dragManager-focused)**: +- Imports `moveItem`, `moveSection` from sectionsStore +- Uses high-level API calls: `moveItem({ itemId, targetSectionId, targetItemId, position, transform })` +- Cleaner separation of concerns - drag logic stays in dragManager, state updates in sectionsStore +- More declarative approach + +**PR #136 (sectionsStore-focused)**: +- Imports `sections`, `updateSections`, `setSections` from sectionsStore +- Uses lower-level store updates: `updateSections((secs) => { /* manual array manipulation */ })` +- More imperative approach with direct array manipulation +- Includes error recovery with backup/restore pattern + +### 2. State Update Pattern + +**PR #133**: +```javascript +// Clean API approach +moveItem({ + itemId: movedItem.id, + targetSectionId: targetSection.id, + targetItemId: targetItem ? targetItem.id : null, + position: state.dropPosition || 'after', + transform: () => prepareTimelineItem(movedItem, state, secs) +}); +``` + +**PR #136**: +```javascript +// Direct store manipulation +updateSections((secs) => { + const newSecs = [...secs]; + // Manual array splicing and manipulation + sectionItems.splice(finalSourceItemIndex, 1); + sectionItems.splice(targetIndex, 0, movedItem); + // Update sections + newSecs[sourceSection] = { ...newSecs[sourceSection], items: sectionItems }; + return newSecs; +}); +``` + +### 3. Error Handling + +**PR #133**: +- Minimal error handling +- Relies on sectionsStore to handle errors +- Uses try-catch and re-throws errors + +**PR #136**: +- Extensive error handling with backup/restore pattern +- Creates full backup before operations: `JSON.parse(JSON.stringify(get(sections)))` +- Restores state on error: `setSections(sectionsBeforeUpdate)` +- More defensive programming + +### 4. Code Complexity + +**PR #133**: +- Shorter, more concise code (~1452 lines) +- Higher-level abstractions +- Less direct manipulation of arrays +- Cleaner separation of drag logic from state logic + +**PR #136**: +- Longer, more verbose code (~1836 lines) +- Lower-level array manipulations +- More detailed error checking +- Mixes drag logic with state update logic + +### 5. sectionsStore.js Changes + +**PR #133**: +- Adds new helper functions: `moveItem`, `moveSection` +- These functions are not shown but presumably handle the state updates +- Cleaner API design + +**PR #136**: +- Exports raw store and update functions: `sections`, `updateSections`, `setSections` +- No new high-level helper functions for drag operations +- Exposes lower-level store access + +## Pros and Cons + +### PR #133 Pros: +- ✅ Cleaner separation of concerns +- ✅ More maintainable code +- ✅ Better encapsulation of state logic +- ✅ Easier to test (can mock moveItem/moveSection) +- ✅ More declarative API + +### PR #133 Cons: +- ❌ Less detailed error handling +- ❌ No backup/restore mechanism + +### PR #136 Pros: +- ✅ Robust error handling with backup/restore +- ✅ More defensive programming +- ✅ Self-contained - all logic visible in dragManager +- ✅ Better error recovery + +### PR #136 Cons: +- ❌ More complex and verbose +- ❌ Violates separation of concerns +- ❌ Harder to maintain and test +- ❌ Direct store manipulation is error-prone +- ❌ Mixing drag logic with state update logic + +## Risk Assessment + +**PR #133 Risk**: Medium +- Depends on proper implementation of moveItem/moveSection +- Less error recovery capability +- Cleaner architecture reduces long-term maintenance risk + +**PR #136 Risk**: Low-Medium +- More defensive but also more complex +- Better error recovery +- Higher complexity increases maintenance burden + +## Implementation Details + +### PR #133's moveItem/moveSection Implementation + +PR #133 includes well-implemented helper functions in sectionsStore.js: + +```javascript +export function moveItem({ itemId, targetSectionId, targetItemId = null, position = 'after', transform }) { + sections.update((secs) => { + // Clean implementation using IDs for stable references + // Includes optional transform function for item modification + // Proper error checking and early returns + }); + addToHistory('MOVE_ITEM', { itemId, targetSectionId, targetItemId, position }, 'Moved item'); +} + +export function moveSection({ sectionId, targetSectionId, position = 'after' }) { + sections.update((secs) => { + // Clean section movement logic + // Proper index calculations and bounds checking + }); + addToHistory('MOVE_SECTION', { sectionId, targetSectionId, position }, 'Moved section'); +} +``` + +These functions provide: +- ID-based references (more stable than indices) +- Clean separation of concerns +- Built-in history tracking +- Optional transform function for item modifications during moves + +## Recommendation + +**Merge PR #133 first** for the following reasons: + +1. **Superior Architecture**: The clean separation between drag logic and state updates is the correct approach +2. **Complete Implementation**: The moveItem/moveSection functions are properly implemented +3. **Better API Design**: ID-based operations are more stable than index-based ones +4. **Maintainability**: The declarative approach will be easier to maintain and extend + +### Rationale: +- PR #133's architecture is superior and more maintainable +- The separation of concerns will make future changes easier +- The declarative API is less error-prone +- Error handling can be added to the moveItem/moveSection functions + +### Enhancement Plan After Merging PR #133: + +1. **Add Error Handling**: + - Wrap the moveItem/moveSection functions with try-catch blocks + - Add validation for IDs before operations + - Return success/failure indicators + +2. **Add Backup/Restore Pattern**: + ```javascript + export function moveItem({ itemId, targetSectionId, targetItemId = null, position = 'after', transform }) { + const backup = get(sections); + try { + sections.update((secs) => { + // existing logic + }); + addToHistory('MOVE_ITEM', { itemId, targetSectionId, targetItemId, position }, 'Moved item'); + } catch (error) { + sections.set(backup); + console.error('Failed to move item:', error); + throw error; + } + } + ``` + +3. **Close PR #136**: The architectural approach in PR #136 is not recommended despite its robust error handling + +### Testing Priority: +1. Test all drag operations thoroughly with PR #133 +2. Verify that the ID-based approach handles edge cases correctly +3. Ensure history tracking works properly +4. Test error scenarios after adding error handling \ No newline at end of file diff --git a/PR_REVIEW_128_USER_SERVICE_PERMISSIONS.md b/PR_REVIEW_128_USER_SERVICE_PERMISSIONS.md new file mode 100644 index 00000000..8257e5c3 --- /dev/null +++ b/PR_REVIEW_128_USER_SERVICE_PERMISSIONS.md @@ -0,0 +1,193 @@ +# PR #128: UserService Permissions - SECURITY CRITICAL REVIEW + +## Overview +This PR implements user role permissions in the UserService class. However, there are **CRITICAL SECURITY ISSUES** that must be addressed before merging. + +## Branch: `tme4ul-codex/update-ticket-17-update-UserService-to-check-permissions` + +## Critical Security Issues Found + +### 1. **No Admin Role Assignment Mechanism** +- The database migration adds a `role` column with default 'user' +- There is NO mechanism to actually assign 'admin' role to any user +- The `ensureUserExists()` method always uses default 'user' role from auth session +- Without a way to set admin roles, the entire permission system is non-functional + +### 2. **Admin Route Protection is Broken** +- `/routes/admin/+layout.server.js` only checks `dev` mode, NOT user roles +- This means admin routes are completely unprotected in production +- Any authenticated user could potentially access admin functions if deployed + +### 3. **No Actual Permission Checking** +- The `isAdmin()` method exists but is never called anywhere +- No middleware or route guards use this method +- Admin endpoints have no authorization checks + +### 4. **Role Not Populated in Auth Session** +- The auth system (Better Auth) doesn't populate user roles +- `hooks.server.js` calls `ensureUserExists()` but doesn't fetch/populate role +- The role field is never available in `event.locals.user` + +### 5. **Insufficient Test Coverage** +- No tests for the new `isAdmin()` method +- No tests for role-based access control +- No integration tests for admin route protection + +## Code Analysis + +### Migration File Issues +```javascript +// migrations/1752865450_add_role_column_to_users.js +export const up = async (db) => { + await db.schema.alterTable('users', (table) => { + table.string('role').defaultTo('user'); + }); +}; +``` +- Only adds column, no way to populate admin roles +- No index on role column for performance + +### UserService Issues +```javascript +async isAdmin(userRole) { + return userRole === 'admin'; +} +``` +- This is just a string comparison +- Should verify against database +- Method is async for no reason + +### Missing Auth Integration +The role needs to be: +1. Fetched from database during auth +2. Added to session/token +3. Available in `event.locals.user.role` +4. Checked in route guards + +## Required Fixes Before Merging + +### 1. Add Admin Role Assignment +```javascript +// Add method to UserService +async setUserRole(userId, role) { + // Validate role is allowed + if (!['user', 'admin'].includes(role)) { + throw new ValidationError('Invalid role'); + } + + // Update user role + const query = 'UPDATE users SET role = $1 WHERE id = $2 RETURNING *'; + const result = await db.query(query, [role, userId]); + + if (result.rows.length === 0) { + throw new NotFoundError('User not found'); + } + + return result.rows[0]; +} +``` + +### 2. Fix Admin Route Protection +```javascript +// src/routes/admin/+layout.server.js +import { userService } from '$lib/server/services/userService'; +import { error } from '@sveltejs/kit'; + +export async function load({ locals }) { + if (!locals.user) { + throw error(401, 'Unauthorized'); + } + + const isAdmin = await userService.isAdmin(locals.user.role); + if (!isAdmin) { + throw error(403, 'Forbidden'); + } +} +``` + +### 3. Populate Role in Auth Session +```javascript +// In hooks.server.js after getting user +if (sessionResult && sessionResult.user) { + // Fetch user with role from database + const dbUser = await userService.getById(sessionResult.user.id); + + event.locals.session = { + ...sessionResult.session, + user: { + ...sessionResult.user, + role: dbUser.role // Add role to session + } + }; + event.locals.user = event.locals.session.user; +} +``` + +### 4. Add Permission Middleware +```javascript +// src/lib/server/auth/permissions.js +export function requireRole(role) { + return async ({ locals }) => { + if (!locals.user) { + throw error(401, 'Unauthorized'); + } + + if (locals.user.role !== role) { + throw error(403, 'Forbidden'); + } + }; +} + +export const requireAdmin = requireRole('admin'); +``` + +### 5. Add Comprehensive Tests +```javascript +// src/lib/server/services/__tests__/userService.test.js +describe('isAdmin', () => { + it('should return true for admin role', async () => { + const result = await userService.isAdmin('admin'); + expect(result).toBe(true); + }); + + it('should return false for user role', async () => { + const result = await userService.isAdmin('user'); + expect(result).toBe(false); + }); + + it('should return false for undefined role', async () => { + const result = await userService.isAdmin(undefined); + expect(result).toBe(false); + }); +}); + +describe('setUserRole', () => { + // Add tests for role assignment +}); +``` + +## Additional Recommendations + +1. **Add Role Management UI**: Create admin interface to manage user roles +2. **Audit Logging**: Log all role changes for security +3. **Role Hierarchy**: Consider more granular permissions beyond just admin/user +4. **Session Invalidation**: Force re-login when roles change +5. **Rate Limiting**: Add rate limiting to admin endpoints + +## Files Changed +- `migrations/1752865450_add_role_column_to_users.js` - Adds role column +- `src/lib/server/services/userService.js` - Adds isAdmin() method +- Missing: Auth integration, route protection, tests + +## Security Impact +**CRITICAL**: This PR introduces a permission system but doesn't actually implement any security. Admin routes remain unprotected and there's no way to assign admin roles. + +## Recommendation +**DO NOT MERGE** until all security issues are addressed. This PR needs significant additional work to be production-ready. + +## Next Steps +1. Implement proper admin role assignment +2. Fix admin route protection +3. Integrate roles with auth system +4. Add comprehensive tests +5. Security review after fixes \ No newline at end of file diff --git a/PR_REVIEW_PLAN.md b/PR_REVIEW_PLAN.md new file mode 100644 index 00000000..cdc9dfe2 --- /dev/null +++ b/PR_REVIEW_PLAN.md @@ -0,0 +1,492 @@ +# QDrill PR Review and Merge Plan + +## Executive Summary +- **Total PRs**: 29 open PRs +- **Critical Conflicts**: PRs #133 and #136 (core state management overlap) +- **Highest Risk**: State refactoring PRs affecting practice plan functionality +- **Estimated Timeline**: 4 weeks for complete review and merge + +## Critical Issues to Resolve + +### 1. Major Conflict: PRs #133 vs #136 [RESOLVED] +**Issue**: Both PRs heavily modified dragManager.js and sectionsStore.js with incompatible approaches +**Resolution**: +- Merged PR #133 (chosen for superior architecture with moveItem/moveSection helper functions) +- Enhanced with error handling and backup/restore patterns from PR #136 +- Closed PR #136 + +### 2. Component Deletion Conflict +**Issue**: PR #125 marks SkeletonLoader.svelte for deletion, but PR #112 enhances it +**Resolution**: ✅ RESOLVED - PR #112 merged first, SkeletonLoader is actively used and should NOT be deleted + +### 3. Missing Test Coverage +**Issue**: Complex refactors lack comprehensive test coverage +**Resolution**: Add tests before merging high-risk PRs + +## Phase-by-Phase Execution Plan + +### Phase 1: Low-Risk Foundation (Days 1-3) +Start with these independent, low-risk PRs: + +#### PR #127 - SQL Duplication Cleanup +**Status**: ✅ MERGED +**Merge Date**: 2025-07-19 +**Known Issues**: See [Filter Panel Errors](/docs/known-issues/filter-panel-errors.md) and [Unit Test Failures](/docs/known-issues/unit-test-failures.md) - these are pre-existing issues not caused by this PR + +```bash +# Review and merge process +gh pr checkout 127 +pnpm install +vercel dev + +# Run tests +pnpm run test:unit --run src/lib/server/services/__tests__ + +# Manual testing +# 1. Navigate to /drills +# 2. Test search functionality +# 3. Test skill filtering +# 4. Verify no SQL errors in console + +# Verification Results: +# ✅ SQL refactoring working correctly +# ✅ No SQL errors in console +# ✅ Search functionality operational +# ✅ Skill filtering functional +# ⚠️ Pre-existing JS errors in FilterPanel (documented) +# ⚠️ Pre-existing unit test failures (documented) + +# If all tests pass +gh pr review 127 --approve +gh pr merge 127 +``` + +#### PR #135 - API Error Handling +**Status**: ✅ MERGED +**Merge Date**: 2025-07-19 +**Branch**: m3sefp-codex/update-11-api-error-handling.md + +```bash +# Review and merge process +gh pr checkout 135 +pnpm install +vercel dev + +# Implementation verified: +# ✅ Created centralized apiFetch utility with comprehensive error handling +# ✅ Network error detection and user-friendly messages +# ✅ HTTP error status handling with appropriate messages +# ✅ JSON parsing error handling +# ✅ Server-side error handling with try-catch blocks + +# Testing results: +# ✅ All pages load successfully with apiFetch integration +# ✅ Form validation errors display correctly +# ✅ API responses handled gracefully +# ✅ Error messages would be user-friendly if errors occurred + +# Files modified: +# - src/lib/utils/apiFetch.js (new centralized error handling) +# - Multiple server files updated with try-catch blocks +# - Client components updated to use apiFetch + +# Merged successfully after resolving conflict in tickets/11-api-error-handling.md +gh pr review 135 --approve +gh pr merge 135 +``` + +#### PR #119 - Vercel Rewrites Config +**Status**: ✅ MERGED +**Merge Date**: 2025-07-19 +**Branch**: lo6xdp-codex/update-33-config-vercel-rewrites.md + +```bash +# Verification completed: +# ✅ Removed redundant rewrites from vercel.json (now just empty object) +# ✅ All API routes work correctly without rewrites +# ✅ Tested /api/drills and /api/practice-plans endpoints +# ✅ SvelteKit filesystem routing handles all paths properly + +# Successfully merged +gh pr review 119 --approve +gh pr merge 119 +``` + +#### PR #120 - Theme Consistency +**Status**: ✅ MERGED +**Merge Date**: 2025-07-19 +**Branch**: 7i2qy3-codex/update-ticket-for-32-config-theme-consistency + +```bash +# Verification completed: +# ✅ CSS variables are single source of truth for colors +# ✅ Tailwind config references CSS variables +# ✅ Hardcoded drag-and-drop colors replaced with CSS vars +# ✅ Theme colors applied correctly (theme-1: #ff3e00, theme-2: #4075a6) +# ✅ Visual consistency maintained + +# Successfully merged +gh pr review 120 --approve +gh pr merge 120 +``` + +### Phase 2: Dead Code Cleanup (Days 4-5) + +#### Order is important to avoid conflicts: + +1. **PR #112 - Loading States Implementation** (Do this FIRST) +**Status**: ✅ MERGED +**Merge Date**: 2025-07-19 +**Branch**: 0anu39-codex/update-ticket-for-loading-states-implementation + +```bash +# Implementation completed and verified: +# ✅ Enhanced SkeletonLoader component with new props (showButton, showCard, height) +# ✅ Added skeleton loaders to drills page during navigation +# ✅ Added skeleton loaders to practice plans page during navigation +# ✅ Implemented loading state for FilterPanel drill search with spinner +# ✅ Added form submission loading states to DrillForm +# ✅ Tested all loading states with browser automation + +# Files modified: +# - src/lib/components/SkeletonLoader.svelte (enhanced with new features) +# - src/routes/drills/+page.svelte (skeleton loaders added) +# - src/routes/practice-plans/+page.svelte (skeleton loaders added) +# - src/lib/components/FilterPanel.svelte (search loading state) +# - src/routes/drills/DrillForm.svelte (form submission loading) + +# Merge notes: +# - Resolved conflict in SkeletonLoader.svelte with main branch +# - Component is actively used and should NOT be deleted by PR #125 +``` + +2. **PR #125 - Dead Code Components** (After confirming SkeletonLoader status) +**Status**: ✅ MERGED +**Merge Date**: 2025-07-19 + +```bash +# Implementation completed and verified: +# ✅ Updated PR to keep SkeletonLoader.svelte (actively used by loading states) +# ✅ Verified remaining component deletions were safe (no imports found) +# ✅ Tested application - all pages load correctly without deleted components +# ✅ Resolved merge conflicts with main branch + +# Components removed: +# - Cart.svelte +# - LoginButton.svelte +# - TitleWithTooltip.svelte +# - ParallelActivityCreator.svelte +# - ParallelTimelineView.svelte +# - DrillSearchModal.svelte + +# Components kept: +# - SkeletonLoader.svelte (enhanced and actively used) +``` + +3. **PR #124 - Dead Code Stores** +**Status**: ✅ MERGED +**Merge Date**: 2025-07-19 + +```bash +# Implementation completed and verified: +# ✅ Removed unused store functions that were not imported anywhere +# ✅ Verified feedbackStore.js is still needed (used by FeedbackModal and FeedbackButton) +# ✅ Tested application functionality - all pages work correctly + +# Functions removed: +# - resetDrillFilters() from drillsStore.js +# - removeTimelineFromGroup() from sectionsStore.js +# - withHistory() from historyStore.js +# - resetPracticePlanFilters() from practicePlanFilterStore.js +# - resetForm() from practicePlanMetadataStore.js +# - initializeWizard() from wizardStore.js + +# Stores kept: +# - feedbackStore.js (actively used) +``` + +4. **PR #123 - Admin Migration Page** +**Status**: ✅ MERGED +**Merge Date**: 2025-07-19 + +```bash +# Implementation completed and verified: +# ✅ Deleted unused admin migration page that called non-existent endpoints +# ✅ No references to admin routes found in the codebase +# ✅ Tested application - returns 404 for /admin as expected + +# Files removed: +# - src/routes/admin/+layout.server.js +# - src/routes/admin/+page.svelte +``` + +5. **PR #122 - Dead Code API Routes** +```bash +gh pr checkout 122 +# Verify no frontend code calls deleted endpoints +``` + +### Phase 3: Core State Refactoring (Days 6-10) + +#### ✅ COMPLETED: Conflict between #133 and #136 resolved + +**Resolution Summary**: +- PR #133 merged successfully with enhanced error handling +- PR #136 closed in favor of #133's superior architecture +- Enhanced moveItem/moveSection functions with: + - Comprehensive error handling with try-catch blocks + - Backup/restore pattern for atomic operations + - Input validation for all parameters + - User-friendly toast notifications on errors + +**Next State Refactoring PR**: + +#### PR #139 - Component Coupling (After state refactor) +```bash +gh pr checkout 139 +git rebase main # Get latest state changes +vercel dev + +# Test all decoupled components still work +# Verify props are passed correctly +``` + +### Phase 4: Component Refactoring (Days 11-13) + +#### PR #131 - PracticePlanForm Refactor +```bash +gh pr checkout 131 +git rebase main # May have conflicts with #139 +vercel dev + +# Test form validation +# Test auth flow +# Test save functionality +# Subagent task: Test all form edge cases +``` + +#### PR #130 - FilterPanel Refactor +```bash +gh pr checkout 130 +vercel dev + +# Test all filter combinations +# Verify performance with large datasets +``` + +#### PR #132 - ExcalidrawWrapper +```bash +gh pr checkout 132 +vercel dev + +# Test diagram creation/editing +# Verify save functionality +``` + +### Phase 5: UX Improvements (Days 14-18) +Can be done in parallel with subagents: + +```bash +# Subagent 1: Handle navigation and accessibility +gh pr checkout 111 # Navigation accessibility +gh pr checkout 126 # Accessibility review + +# Subagent 2: Handle loading and error states +gh pr checkout 112 # Loading states (if not already done) +gh pr checkout 114 # Error handling UX + +# Subagent 3: Handle filter and search improvements +gh pr checkout 116 # Clear/reset filters +gh pr checkout 129 # Search experience + +# Subagent 4: Handle remaining UX +gh pr checkout 113 # Landing page CTA +gh pr checkout 115 # Empty states +gh pr checkout 117 # Add to plan unauthenticated +gh pr checkout 121 # Practice plan position filter +``` + +### Phase 6: Final Items (Days 19-20) + +#### PR #128 - UserService Permissions +```bash +gh pr checkout 128 +vercel dev + +# Critical security testing +# 1. Test as regular user - verify no admin access +# 2. Test as admin - verify admin features work +# 3. Test permission edge cases +# Subagent task: Security audit of all permission checks +``` + +#### PR #134 - Validation Framework +```bash +gh pr checkout 134 +# Test all validation rules +# Verify error messages are helpful +``` + +#### PR #118 - LLM Practice Plan Tools +```bash +gh pr checkout 118 +# Test wrapper scripts work correctly +``` + +## Automated Testing Script + +Create this script for each PR: +```bash +#!/bin/bash +# test-pr.sh + +PR_NUMBER=$1 +echo "Testing PR #$PR_NUMBER" + +# Checkout and setup +gh pr checkout $PR_NUMBER +pnpm install + +# Run linting +echo "Running lint checks..." +pnpm run lint +if [ $? -ne 0 ]; then + echo "Lint failed!" + exit 1 +fi + +# Run type checking +echo "Running type checks..." +pnpm run check +if [ $? -ne 0 ]; then + echo "Type check failed!" + exit 1 +fi + +# Run unit tests +echo "Running unit tests..." +pnpm run test:unit:run +if [ $? -ne 0 ]; then + echo "Unit tests failed!" + exit 1 +fi + +# Start dev server in background +vercel dev & +DEV_PID=$! +sleep 10 + +# Run Playwright tests +echo "Running Playwright tests..." +pnpm run test +TEST_RESULT=$? + +# Cleanup +kill $DEV_PID + +if [ $TEST_RESULT -ne 0 ]; then + echo "Playwright tests failed!" + exit 1 +fi + +echo "All tests passed for PR #$PR_NUMBER" +``` + +## Subagent Task Assignments + +### Subagent 1: Conflict Resolution Analyst +**Task**: Analyze PRs #133 and #136 to recommend best approach +``` +1. Compare both refactoring approaches +2. List pros/cons of each +3. Identify which requires less rework of other components +4. Recommend merge strategy +``` + +### Subagent 2: Dead Code Verifier +**Task**: Ensure safe deletion of components/stores/routes +``` +1. For each file marked for deletion, search entire codebase for imports +2. Verify no dynamic imports or string references +3. Check for any route references in navigation +4. Create safety report +``` + +### Subagent 3: Test Suite Builder +**Task**: Create comprehensive Playwright tests for high-risk PRs +``` +1. Write detailed test scenarios for practice plan creation/editing +2. Create drag-and-drop test suite +3. Build permission testing scenarios +4. Generate visual regression tests +``` + +### Subagent 4: UX Testing Coordinator +**Task**: Batch test all UX improvement PRs +``` +1. Test each UX PR for functionality +2. Verify no regressions in user flows +3. Check mobile responsiveness +4. Document any issues found +``` + +### Subagent 5: Security Auditor +**Task**: Review permission and auth-related changes +``` +1. Audit PR #128 for security vulnerabilities +2. Verify all endpoints check permissions +3. Test edge cases in auth flow +4. Review session handling +``` + +## Risk Mitigation Strategies + +1. **Before each merge**: + - Run full test suite + - Deploy to preview environment + - Get manual QA approval + - Have rollback plan ready + +2. **For high-risk PRs**: + - Pair review with another developer + - Extra testing in staging + - Monitor error rates after deploy + - Be ready to revert + +3. **Conflict resolution**: + - Always rebase on latest main + - Test thoroughly after rebasing + - Document any changes made during rebase + +## Success Metrics + +- [ ] All 29 PRs reviewed and merged/closed +- [ ] No production incidents from merges +- [ ] Test coverage increased +- [ ] Performance metrics maintained or improved +- [ ] No accessibility regressions + +## Daily Checklist + +1. Pull latest main: `git checkout main && git pull` +2. Check PR status: `gh pr list` +3. Run test suite on main: `./test-pr.sh main` +4. Review and merge 2-3 PRs following plan +5. Update this document with progress +6. Communicate blockers immediately + +## Emergency Procedures + +If a merge causes issues: +1. Revert immediately: `gh pr revert ` +2. Notify team +3. Create hotfix if needed +4. Update test suite to catch issue +5. Re-attempt with fixes + +## Communication Plan + +- Daily: Update PR status in team channel +- On conflicts: Tag PR authors for clarification +- On completion: Summary of changes and impacts +- Weekly: Progress report on overall plan \ No newline at end of file diff --git a/README.md b/README.md index 4d3b8e37..14bbaa21 100644 --- a/README.md +++ b/README.md @@ -1,36 +1,239 @@ -# SvelteKit Demo app +# QDrill -The official demo app for SvelteKit, hosted on Vercel. +A comprehensive web-based application for sports drill management and practice planning. -## Deploy Your Own +## Overview -[![Deploy with Vercel](https://vercel.com/button)](https://vercel.com/new/clone?repository-url=https%3A%2F%2Fgithub.com%2Fvercel%2Fvercel%2Ftree%2Fmain%2Fexamples%2Fsveltekit-1&project-name=sveltekit-vercel&repository-name=sveltekit-vercel&demo-title=SvelteKit%20%2B%20Vercel&demo-url=https%3A%2F%2Fsveltekit-template.vercel.app%2F) +QDrill is a web-based application designed to be a sports drill bank and practice planning tool for a niche sport. The application allows users to create, manage, and share drills, as well as plan practices. The user experience (UX) is a high priority, with an emphasis on smooth, responsive interactions and a modern design. The application is expected to handle up to a few hundred concurrent users and is built with SvelteKit for both frontend and backend. -_Live Example: https://sveltekit-1-template.vercel.app_ +## Technology Stack -## Developing +- **Frontend**: Svelte with SvelteKit +- **Backend**: SvelteKit +- **Database**: Neon (PostgreSQL) +- **Authentication**: Auth.js (using Google OAuth) +- **Diagramming**: Excalidraw (via `@excalidraw/excalidraw`, note: includes React dependency) +- **Rich Text Editing**: TinyMCE +- **Deployment**: Hosted on Vercel +- **CSS**: Tailwind CSS +- **Testing**: Vitest (unit), Playwright and Cypress (end-to-end) -Once you've created a project and installed dependencies with `npm install` (or `pnpm install` or `yarn`), start a development server: +## Core Features -```bash -npm run dev +### 1. Drill and Formation Management -# or start the server and open the app in a new browser tab -npm run dev -- --open -``` +- **Form-Based Drill Creation**: Users can create new drills via a form interface. Each drill will have the following attributes: + - Name (required) + - Brief description (required) + - How to teach it/detailed description + - Skill level required (required) + - Complexity to explain + - Suggested length of time (required) + - Number of people required + - Skills focused on (required) + - Positions focused on (required) + - Video link to drill + - Images of drill +- **Dynamic URL Generation**: Each drill will automatically be assigned a unique URL upon creation, allowing users to share and access drills directly. +- **Public vs. Private Drills**: Users can choose to make drills public or private. Public drills are accessible by all, while private drills require a specific link. Users can also create public versions of private drills with a different description. -## Building +### 2. Drill and Formation Filtering and Viewing -To create a production version of your app: +- **Client-Side Filtering**: Drills and formations are currently fetched to the client-side, where filtering occurs. While this provides a responsive feel for smaller datasets, it has been identified as a scalability limitation for larger numbers of items (code review notes). +- **Listing Views**: The main views will display lists of drills or formations, showing their name, attributes, brief description, and indications of any media (pictures/videos). +- **Detail Pages**: Each drill and formation will have a dedicated page showing all of its details. Users can comment on drills, upvote them, or create variations. +- **Formations**: A specialized system for static player positions that can be created, shared, and viewed separately from the dynamic drills. -```bash -npm run build -``` +### 3. Practice Plan Creation and Management -You can preview the production build with `npm run preview`. +- **Form-Based Practice Planning**: Users can create practice plans by selecting drills based on the number of players, skill levels, practice duration, and skills to focus on. Two primary methods exist: a cart-based approach (adding drills to a cart first) and a step-by-step wizard. +- **Plan Customization**: After selecting drills, users can define additional practice details, including: + - Practice name + - Practice goals + - Phase of the season + - Number of participants suited for + - Level of experience suited for + - Skills focused on + - Brief overview of practice flow + - Time dedicated to each drill + - Breaks between drills + - Total practice time +- **Public vs. Private Plans**: Similar to drills, practice plans can be published either privately (accessible via link) or publicly. Users can write different overviews for public/private versions if desired. -## Speed Insights +### 4. User Interaction and Profiles -Once deployed on Vercel, you can benefit from [Speed Insights](https://vercel.com/docs/concepts/speed-insights) simply by navigating to Vercel's dashboard, clicking on the 'Speed Insights' tab, and enabling the product. +- **User Accounts**: Users can create accounts via OAuth (e.g., Google). Logged-in users can create, comment on, and upvote drills or practice plans, and create variations. +- **Anonymous Interaction**: Users who are not logged in can still view and vote on drills, and create practice plans, though publishing or saving them for future editing requires logging in. +- **User Profiles**: Profiles can include optional information such as name, team played for, country, and social media links. +- **User Access**: All users will have the same level of access. Each user will have their own private drills and practice plans, and they can save public drills and plans. Users can see the drills and plans they have saved. -You will get data once your application will be re-deployed and will receive visitors. +### 5. Backend and Data Management + +- **Vercel Postgres Database**: Used for storing all application data, including drills, practice plans, user accounts, and comments. +- **API Integration**: The SvelteKit frontend will communicate with the SvelteKit backend via RESTful APIs to manage drill creation, filtering, user management, and more. +- **Media Management**: Images will be hosted directly on the application, while videos will be linked from external sources (e.g., YouTube, cloud storage). + +### 6. Deployment and Hosting + +- **Frontend**: Hosted on Vercel with the custom domain (e.g., qdrill.app). +- **Backend**: Hosted on Vercel's serverless functions or as a separate service if needed, handling API requests and database interactions. + +### 7. Testing and Quality Assurance + +- **Vitest**: Used for unit testing the service layer and other backend functionality. +- **Playwright**: Used for end-to-end testing. +- **Cypress**: Used for additional end-to-end testing to ensure the entire user flow, from drill creation to practice plan publishing, works smoothly. + +### 8. UX and Design + +- **Design Aesthetic**: The design will follow a style similar to Figma, with a clean, minimalist look. The color scheme and fonts will be inspired by Figma, but with an emphasis on ensuring faster loading times and responsive filtering without noticeable delays. + +## Development + +### Getting Started + +1. **Install dependencies**: + + ```bash + pnpm install + ``` + +2. **Run the development server**: + + ```bash + vercel dev + ``` + +3. **Check TypeScript + SvelteKit sync**: + ```bash + pnpm run check + ``` + +### Package Management + +- **Install dependencies**: + + ```bash + pnpm install + ``` + +- **Add a package**: + ```bash + pnpm add + ``` + +### Testing + +- **Run Playwright tests**: + + ```bash + pnpm run test + ``` + +- **Run a specific test**: + + ```bash + pnpm test -- tests/test.js + ``` + +- **Run Vitest unit tests in watch mode**: + + ```bash + pnpm run test:unit + ``` + +- **Run Vitest unit tests once**: + + ```bash + pnpm run test:unit:run + ``` + +- **Run Vitest unit tests with coverage**: + ```bash + pnpm run test:unit:coverage + ``` + +### Code Quality + +- **Run linting checks**: + + ```bash + pnpm run lint + ``` + +- **Fix formatting issues**: + ```bash + pnpm run format + ``` + +### Database Migrations + +Database schema changes are managed using `node-pg-migrate`. + +- **Create a new migration**: + + ```bash + npx node-pg-migrate create + ``` + + Replace `` with a descriptive name (e.g., `add_user_email_column`). + +- **Run migrations**: + + ```bash + npx node-pg-migrate up + ``` + + This applies all pending migrations. Make sure your `DATABASE_URL` environment variable is set correctly (e.g., in `.env.local` or your shell environment). + +- **Rollback the last migration**: + ```bash + npx node-pg-migrate down + ``` + +### Deployment + +Deployment is automatic from the GitHub main branch to Vercel. + +## Documentation + +Project documentation is organized in two main locations: + +1. **`docs/`**: Detailed technical documentation + - **`Architecture`**: System design, patterns, and architectural decisions + - **`Implementation`**: Technical details and implementation specifics + - Drag and Drop System + - Timeline Management + - Service Layer Architecture +2. **`code-review/`**: Contains detailed findings and notes from a comprehensive code review conducted to assess codebase health, identify areas for improvement, and align with professional development standards. See `code-review/holistic-summary.md` for an overview. + +### Documentation Workflow + +When making changes to the codebase: + +1. First examine `/docs/index.md` to understand the documentation structure +2. Navigate to the appropriate subdirectory based on the nature of your changes: + - `/docs/architecture/` for architectural changes or patterns + - `/docs/implementation/` for implementation details and technical references +3. Update existing documentation files or create new ones as needed +4. Update index files to reference any new documentation + +### Documentation Requirements + +- Create/update documentation when modifying .js/.svelte files +- Document component descriptions, usage instructions, and relationships +- Maintain documentation consistency for directory structure +- Consider component interdependencies when making changes +- Follow best practices for Svelte documentation + +## Code Style Guidelines + +- **AI-Readability**: Add clear comments to make code easily understood by future AI systems +- **Comments**: Include purpose explanations, input/output expectations, and logic clarifications +- **Imports**: Group imports by source (svelte, lib, components) +- **Components**: Use Svelte components with script/markup/style structure +- **Stores**: Use reactive declarations with $ prefix for store values +- **Error Handling**: Use try/catch with specific error messages +- **API Endpoints**: Return standardized JSON responses with proper status codes +- **Database**: Use parameterized queries to prevent SQL injection +- **Naming**: Use descriptive camelCase for variables/functions, PascalCase for components diff --git a/code-review/api-auth-notes.md b/code-review/api-auth-notes.md new file mode 100644 index 00000000..a230939b --- /dev/null +++ b/code-review/api-auth-notes.md @@ -0,0 +1,4 @@ +# API Auth Route Notes + +- `src/routes/api/auth/[...auth]/+server.js`: +- `src/routes/api/auth/actions/`: diff --git a/code-review/api-comments-notes.md b/code-review/api-comments-notes.md new file mode 100644 index 00000000..58a44978 --- /dev/null +++ b/code-review/api-comments-notes.md @@ -0,0 +1,3 @@ +# API Comments Route Notes + +- `src/routes/api/comments/+server.js`: diff --git a/code-review/api-feedback-notes.md b/code-review/api-feedback-notes.md new file mode 100644 index 00000000..60535a81 --- /dev/null +++ b/code-review/api-feedback-notes.md @@ -0,0 +1,5 @@ +# API Feedback Route Notes + +- `src/routes/api/feedback/+server.js`: +- `src/routes/api/feedback/[id]/delete/+server.js`: +- `src/routes/api/feedback/[id]/upvote/+server.js`: diff --git a/code-review/api-poll-notes.md b/code-review/api-poll-notes.md new file mode 100644 index 00000000..5efdbc1b --- /dev/null +++ b/code-review/api-poll-notes.md @@ -0,0 +1,4 @@ +# API Poll Route Notes + +- `src/routes/api/poll/+server.js`: +- `src/routes/api/poll/options/+server.js`: diff --git a/code-review/api-tests-notes.md b/code-review/api-tests-notes.md new file mode 100644 index 00000000..ecedc0eb --- /dev/null +++ b/code-review/api-tests-notes.md @@ -0,0 +1,6 @@ +# API Tests Notes + +- `src/routes/api/__tests__/drill-id.test.js`: +- `src/routes/api/__tests__/drills.test.js`: +- `src/routes/api/__tests__/practice-plan-id.test.js`: +- `src/routes/api/__tests__/practice-plans.test.js`: diff --git a/code-review/api-votes-notes.md b/code-review/api-votes-notes.md new file mode 100644 index 00000000..73bba841 --- /dev/null +++ b/code-review/api-votes-notes.md @@ -0,0 +1,4 @@ +# API Votes Route Notes + +- `src/routes/api/votes/+server.js`: +- `src/routes/api/votes/user/+server.js`: diff --git a/code-review/code-review.md b/code-review/code-review.md new file mode 100644 index 00000000..4d472d91 --- /dev/null +++ b/code-review/code-review.md @@ -0,0 +1,455 @@ +# Code Review + +This document tracks a code review focused on identifying areas for improvement in Svelte/SvelteKit practices, component structure, state management, API design, and overall code quality to align with professional development standards. The goal is to pinpoint patterns or practices that might distinguish the codebase from that of a seasoned Svelte developer. Files are marked with ✅ as they are reviewed, with notes summarizing findings and potential issues put into their own notes files, which are created and grouped as needed, and noted at the bottom of this file. + +If they have ✅\*, they have been marked as not worth reviewing + +When reviewing, first create a new notes file, add your reviews there, then update the main doc here to mark what you reviewed. + +## Project Structure + +``` +. +├── README.md +├── code-review/ +│ ├── code-review.md +│ ├── config-app-notes.md +│ ├── diagram-notes.md +│ ├── drill-notes.md +│ ├── library-notes.md +│ ├── modal-notes.md +│ ├── practice-plan-notes.md +│ ├── practice-plan-wizard-notes.md +│ ├── service-notes.md +│ └── shared-components-notes.md +├── coverage/ ✅* +├── cypress/ ✅* +│ ├── component/ ✅* +│ │ └── drillitem.cy.js ✅* +│ ├── downloads/ ✅* +│ ├── e2e/ ✅* +│ │ ├── README.md ✅* +│ │ ├── drills_list.cy.js ✅* +│ │ ├── formation_detail.cy.js ✅* +│ │ ├── formations_list.cy.js ✅* +│ │ ├── pagination.cy.js ✅* +│ │ ├── sorting.cy.js ✅* +│ │ └── visibility.cy.js ✅* +│ ├── fixtures/ ✅* +│ │ └── example.json ✅* +│ ├── screenshots/ ✅* +│ └── support/ ✅* +│ ├── commands.js ✅* +│ ├── component-index.html ✅* +│ ├── component.js ✅* +│ └── e2e.js ✅* +├── cypress.config.js ✅* +├── docs/ +│ ├── architecture/ +│ │ └── index.md +│ ├── implementation/ +│ │ ├── drag-and-drop.md +│ │ ├── index.md +│ │ ├── service-layer.md +│ │ └── timeline-management.md +│ └── index.md +├── drill-banks/ ✅* +│ └── canada3.csv ✅* +├── jsconfig.json ✅* +├── migrations/ ✅* +│ ├── 1744523609140_initial-schema.js ✅* +│ └── 1744527001396_add-performance-indexes.js ✅* +├── package.json ✅* +├── playwright.config.js ✅* +├── pnpm-lock.yaml ✅* +├── pnpm-workspace.yaml ✅* +├── postcss.config.cjs ✅* +├── scripts/ ✅* +│ └── har_cleaner.py ✅* +├── src/ +│ ├── app.css ✅* +│ ├── app.d.ts ✅* +│ ├── app.html ✅ +│ ├── components/ +│ │ ├── Breadcrumb.svelte ✅ +│ │ ├── Cart.svelte ✅ +│ │ ├── Comments.svelte ✅ +│ │ ├── DeletePracticePlan.svelte ✅ +│ │ ├── ExcalidrawWrapper.svelte ✅ +│ │ ├── FeedbackButton.svelte ✅ +│ │ ├── FeedbackModal.svelte ✅ +│ │ ├── FilterPanel.svelte ✅ +│ │ ├── LoginButton.svelte ✅ +│ │ ├── Spinner.svelte ✅ +│ │ ├── ThreeStateCheckbox.svelte ✅ +│ │ ├── UpvoteDownvote.svelte ✅ +│ │ └── practice-plan/ +│ │ ├── items/ +│ │ │ ├── DrillItem.svelte ✅ +│ │ │ ├── ParallelGroup.svelte ✅ +│ │ │ └── TimelineColumn.svelte ✅ +│ │ ├── modals/ +│ │ │ ├── DrillSearchModal.svelte ✅ +│ │ │ ├── EmptyCartModal.svelte ✅ +│ │ │ └── TimelineSelectorModal.svelte ✅ +│ │ ├── sections/ +│ │ │ │ ├── SectionContainer.svelte ✅ +│ │ │ └── wizard/ +│ │ │ │ ├── +layout.svelte ✅ +│ │ │ │ ├── +page.server.js ✅ +│ │ │ │ ├── +page.svelte ✅ +│ │ │ │ ├── basic-info/ +│ │ │ │ │ └── +page.svelte ✅ +│ │ │ │ ├── drills/ +│ │ │ │ │ ├── +page.server.js ✅ +│ │ │ │ │ └── +page.svelte ✅ +│ │ │ │ ├── overview/ +│ │ │ │ │ └── +page.svelte ✅ +│ │ │ │ ├── sections/ +│ │ │ │ │ └── +page.svelte ✅ +│ │ │ │ └── timeline/ +│ │ │ │ │ └── +page.svelte ✅ +│ │ │ └── styles.css ✅ +│ ├── hooks.server.js ✅ +│ ├── lib/ ✅ +│ │ ├── __mocks__/ ✅* +│ │ │ ├── environment.js ✅* +│ │ │ ├── navigation.js ✅* +│ │ │ └── stores.js ✅* +│ │ ├── constants/ ✅ +│ │ │ └── skills.js ✅ +│ │ ├── constants.js ✅ +│ │ ├── images/ ✅* +│ │ │ ├── github.svg ✅* +│ │ │ ├── svelte-logo.svg ✅* +│ │ │ ├── svelte-welcome.png ✅* +│ │ │ └── svelte-welcome.webp ✅* +│ │ ├── server/ ✅ +│ │ │ ├── __mocks__/ ✅* +│ │ │ │ └── db.js ✅* +│ │ │ ├── __tests__/ ✅* +│ │ │ │ └── mocks/ ✅* +│ │ │ │ ├── authGuard.js ✅* +│ │ │ │ └── db.js ✅* +│ │ │ ├── auth.js ✅ +│ │ │ ├── authGuard.js ✅ +│ │ │ ├── db.js ✅ +│ │ │ ├── feedback.js ✅ (Unused?) +│ │ │ └── services/ ✅ +│ │ │ ├── __tests__/ ✅* +│ │ │ │ ├── baseEntityService.test.js ✅* +│ │ │ │ ├── drillService.test.js ✅* +│ │ │ │ ├── formationService.test.js ✅* +│ │ │ │ ├── practicePlanService.test.js ✅* +│ │ │ │ ├── skillService.test.js ✅* +│ │ │ │ └── userService.test.js ✅* +│ │ │ ├── baseEntityService.js ✅ +│ │ │ ├── drillService.js ✅ +│ │ │ ├── formationService.js ✅ +│ │ │ ├── practicePlanService.js ✅ +│ │ │ ├── skillService.js ✅ +│ │ │ └── userService.js ✅ +│ │ ├── stores/ ✅ +│ │ │ ├── __tests__/ ✅* +│ │ │ │ └── dragManager.test.js ✅* +│ │ │ ├── cartStore.js ✅ +│ │ │ ├── dragManager.js ✅ +│ │ │ ├── drillsStore.js ✅ +│ │ │ ├── feedbackStore.js ✅ (only modal visibility) +│ │ │ ├── formationsStore.js ✅ +│ │ │ ├── historyStore.js ✅ +│ │ │ ├── practicePlanStore.js ✅ +│ │ │ ├── sectionsStore.js ✅ +│ │ │ ├── sortStore.js ✅ +│ │ │ ├── wizardStore.js ✅ +│ │ │ └── wizardValidation.js ✅ +│ │ ├── utils/ ✅ +│ │ │ ├── diagramMigration.js ✅ +│ │ │ └── loggerUtils.js ✅ (Unused?) +│ │ └── vitals.js ✅ (Unused?/Disabled?) +│ ├── routes/ +│ │ ├── +layout.server.js ✅ +│ │ ├── +layout.svelte ✅ +│ │ ├── +page.js ✅ +│ │ ├── +page.svelte ✅ +│ │ ├── Counter.svelte ✅ +│ │ ├── Header.svelte ✅ +│ │ ├── about/ ✅ +│ │ │ └── +page.svelte ✅ +│ │ ├── admin/ ✅ +│ │ │ ├── +layout.server.js ✅ +│ │ │ └── +page.svelte ✅ +│ │ ├── api/ +│ │ │ ├── __tests__/ ✅ +│ │ │ │ ├── drill-id.test.js ✅ +│ │ │ │ ├── drills.test.js ✅ +│ │ │ │ ├── practice-plan-id.test.js ✅ +│ │ │ │ └── practice-plans.test.js ✅ +│ │ │ ├── auth/ ✅ +│ │ │ │ ├── [...auth]/ ✅ +│ │ │ │ └── actions/ ✅ +│ │ │ ├── comments/ ✅ +│ │ │ │ └── +server.js ✅ +│ │ │ ├── drill-assets/ ✅ +│ │ │ ├── drills/ ✅ +│ │ │ │ ├── +server.js ✅ +│ │ │ │ ├── [id]/ ✅ +│ │ │ │ │ ├── +server.js ✅ +│ │ │ │ │ ├── associate/ ✅ +│ │ │ │ │ │ └── +server.js ✅ +│ │ │ │ │ ├── set-variant/ ✅ +│ │ │ │ │ │ └── +server.js ✅ +│ │ │ │ │ ├── upvote/ ✅ +│ │ │ │ │ │ └── +server.js ✅ +│ │ │ │ │ └── variations/ ✅ +│ │ │ │ │ └── +server.js ✅ +│ │ │ │ ├── associate/ ✅ +│ │ │ │ │ └── +server.js ✅ +│ │ │ │ ├── bulk-upload/ ✅ +│ │ │ │ │ └── +server.js ✅ +│ │ │ │ ├── filter-options/ ✅ +│ │ │ │ │ └── +server.js ✅ +│ │ │ │ ├── import/ ✅ +│ │ │ │ │ └── +server.js ✅ +│ │ │ │ ├── migrate-diagrams/ ✅ +│ │ │ │ │ └── +server.js ✅ +│ │ │ │ ├── names/ ✅ +│ │ │ │ │ └── +server.js ✅ +│ │ │ │ ├── search/ ✅ +│ │ │ │ │ └── +server.js ✅ +│ │ │ │ └── test-migration/ ✅ +│ │ │ │ └── +server.js ✅ +│ │ │ ├── feedback/ ✅ +│ │ │ │ ├── +server.js ✅ +│ │ │ │ └── [id]/ ✅ +│ │ │ │ ├── delete/ ✅ +│ │ │ │ │ └── +server.js ✅ +│ │ │ │ └── upvote/ ✅ +│ │ │ │ └── +server.js ✅ +│ │ │ ├── formations/ ✅ +│ │ │ │ ├── +server.js ✅ +│ │ │ │ ├── [id]/ +│ │ │ │ │ ├── +server.js ✅ +│ │ │ │ │ └── edit/ +│ │ │ │ │ ├── +page.server.js ✅ +│ │ │ │ │ └── +page.svelte ✅ +│ │ │ │ └── search/ +│ │ │ │ └── +server.js ✅ +│ │ │ ├── poll/ ✅ +│ │ │ │ ├── +server.js ✅ +│ │ │ │ └── options/ ✅ +│ │ │ │ └── +server.js ✅ +│ │ │ ├── practice-plans/ ✅ +│ │ │ │ ├── +server.js ✅ +│ │ │ │ └── [id]/ ✅ +│ │ │ │ ├── +server.js ✅ +│ │ │ │ ├── associate/ ✅ +│ │ │ │ │ └── +server.js ✅ +│ │ │ │ └── duplicate/ ✅ +│ │ │ │ └── +server.js ✅ +│ │ │ ├── skills/ ✅ +│ │ │ │ └── +server.js ✅ +│ │ │ ├── users/ ✅ +│ │ │ │ └── me/ ✅ +│ │ │ │ └── +server.js ✅ +│ │ │ └── votes/ ✅ +│ │ │ ├── +server.js ✅ +│ │ │ └── user/ ✅ +│ │ │ └── +server.js ✅ +│ │ ├── auth/ ✅ +│ │ │ ├── [...auth]/ ✅ +│ │ │ └── error/ ✅ +│ │ │ └── +page.svelte ✅ +│ │ ├── drills/ ✅ +│ │ │ ├── +page.server.js ✅ +│ │ │ ├── +page.svelte ✅ +│ │ │ ├── DrillForm.svelte ✅ +│ │ │ ├── [id]/ ✅ +│ │ │ │ ├── +page.server.js ✅ +│ │ │ │ ├── +page.svelte ✅ +│ │ │ │ └── edit/ ✅ +│ │ │ │ ├── +page.server.js ✅ +│ │ │ │ └── +page.svelte ✅ +│ │ │ ├── bulk-upload/ ✅ +│ │ │ │ ├── +page.server.js ✅ +│ │ │ │ └── +page.svelte ✅ +│ │ │ └── create/ ✅ +│ │ │ ├── +page.server.js ✅ +│ │ │ └── +page.svelte ✅ +│ │ ├── feedback/ ✅ +│ │ │ ├── +page.server.js ✅ +│ │ │ └── +page.svelte ✅ +│ │ ├── formations/ ✅ +│ │ │ ├── +page.server.js ✅ +│ │ │ ├── +page.svelte ✅ +│ │ │ ├── FormationForm.svelte ✅ +│ │ │ ├── [id]/ +│ │ │ │ ├── +page.server.js ✅ +│ │ │ │ └── edit/ +│ │ │ │ ├── +page.server.js ✅ +│ │ │ │ └── +page.svelte ✅ +│ │ │ └── create/ +│ │ │ └── +page.svelte ✅ +│ │ ├── poll/ ✅ +│ │ │ ├── +page.server.js ✅ +│ │ │ └── +page.svelte ✅ +│ │ ├── practice-plans/ ✅ +│ │ │ ├── +page.server.js ✅ +│ │ │ ├── +page.svelte ✅ +│ │ │ ├── PracticePlanForm.svelte ✅ +│ │ │ ├── [id]/ ✅ +│ │ │ │ ├── +page.server.js ✅ +│ │ │ │ ├── +page.svelte ✅ +│ │ │ │ └── edit/ ✅ +│ │ │ │ ├── +page.server.js ✅ +│ │ │ │ └── +page.svelte ✅ +│ │ │ ├── actions/ ✅* +│ │ │ ├── components/ ✅* +│ │ │ ├── create/ ✅ +│ │ │ │ ├── +page.server.js ✅ +│ │ │ │ └── +page.svelte ✅ +│ │ │ ├── stores/ ✅* +│ │ │ ├── utils/ ✅* +│ │ │ ├── viewer/ ✅ +│ │ │ │ ├── DrillCard.svelte ✅ +│ │ │ │ ├── ParallelGroup.svelte ✅ +│ │ │ │ ├── Section.svelte ✅ +│ │ │ │ └── Timeline.svelte ✅ +│ │ │ └── wizard/ +│ │ │ ├── +layout.svelte ✅ +│ │ │ ├── +page.server.js ✅ +│ │ │ ├── +page.svelte ✅ +│ │ │ ├── basic-info/ +│ │ │ │ └── +page.svelte ✅ +│ │ │ ├── drills/ +│ │ │ │ ├── +page.server.js ✅ +│ │ │ │ └── +page.svelte ✅ +│ │ │ ├── overview/ +│ │ │ │ └── +page.svelte ✅ +│ │ │ ├── sections/ +│ │ │ │ └── +page.svelte ✅ +│ │ │ └── timeline/ +│ │ │ └── +page.svelte ✅ +│ │ ├── privacy-policy/ ✅ +│ │ │ └── +page.svelte ✅ +│ │ ├── profile/ ✅ +│ │ │ ├── +page.server.js ✅ +│ │ │ └── +page.svelte ✅ +│ │ └── terms-of-service/ ✅ +│ │ └── +page.svelte ✅ +│ └── styles.css ✅ +├── static/ ✅* +│ ├── favicon.png ✅* +│ ├── images/ ✅* +│ │ ├── black-and-white-logo.jpeg ✅* +│ │ ├── bludger.webp ✅* +│ │ ├── cone.webp ✅* +│ │ ├── drill.png ✅* +│ │ ├── favicon.png ✅* +│ │ ├── homepage-hero.jpg ✅* +│ │ ├── icons/ ✅* +│ │ │ ├── b-and-w-player-b1.png ✅* +│ │ │ ├── b-and-w-player-b2.png ✅* +│ │ │ ├── b-and-w-player-c1.png ✅* +│ │ │ ├── b-and-w-player-c2.png ✅* +│ │ │ ├── b-and-w-player-c3.png ✅* +│ │ │ ├── b-and-w-player-k.png ✅* +│ │ │ ├── b-and-w-player-s.png ✅* +│ │ │ ├── bludger.png ✅* +│ │ │ ├── blue-player-b1.png ✅* +│ │ │ ├── blue-player-b2.png ✅* +│ │ │ ├── blue-player-c1.png ✅* +│ │ │ ├── blue-player-c2.png ✅* +│ │ │ ├── blue-player-c3.png ✅* +│ │ │ ├── blue-player-k.png ✅* +│ │ │ ├── blue-player-s.png ✅* +│ │ │ ├── canada-player-b1.png ✅* +│ │ │ ├── canada-player-b2.png ✅* +│ │ │ ├── canada-player-c1.png ✅* +│ │ │ ├── canada-player-c2.png ✅* +│ │ │ ├── canada-player-c3.png ✅* +│ │ │ ├── canada-player-k.png ✅* +│ │ │ ├── canada-player-s.png ✅* +│ │ │ ├── flag.png ✅* +│ │ │ ├── hoops.png ✅* +│ │ │ ├── quaffle.png ✅* +│ │ │ ├── red-black-player-b1.png ✅* +│ │ │ ├── red-black-player-b2.png ✅* +│ │ │ ├── red-black-player-c1.png ✅* +│ │ │ ├── red-black-player-c2.png ✅* +│ │ │ ├── red-black-player-c3.png ✅* +│ │ │ ├── red-black-player-k.png ✅* +│ │ │ ├── red-black-player-s.png ✅* +│ │ │ ├── red-player-b1.png ✅* +│ │ │ ├── red-player-b2.png ✅* +│ │ │ ├── red-player-c1.png ✅* +│ │ │ ├── red-player-c2.png ✅* +│ │ │ ├── red-player-c3.png ✅* +│ │ │ ├── red-player-k.png ✅* +│ │ │ ├── red-player-s.png ✅* +│ │ │ ├── ubc-player-b1.png ✅* +│ │ │ ├── ubc-player-b2.png ✅* +│ │ │ ├── ubc-player-c1.png ✅* +│ │ │ ├── ubc-player-c2.png ✅* +│ │ │ ├── ubc-player-c3.png ✅* +│ │ │ ├── ubc-player-k.png ✅* +│ │ │ ├── ubc-player-s.png ✅* +│ │ │ ├── y-and-b-player-b1.png ✅* +│ │ │ ├── y-and-b-player-b2.png ✅* +│ │ │ ├── y-and-b-player-c1.png ✅* +│ │ │ ├── y-and-b-player-c2.png ✅* +│ │ │ ├── y-and-b-player-c3.png ✅* +│ │ │ ├── y-and-b-player-k.png ✅* +│ │ │ ├── y-and-b-player-s.png ✅* +│ │ │ ├── yellow-arrow-player-b1.png ✅* +│ │ │ ├── yellow-arrow-player-b2.png ✅* +│ │ │ ├── yellow-arrow-player-c1.png ✅* +│ │ │ ├── yellow-arrow-player-c2.png ✅* +│ │ │ ├── yellow-arrow-player-c3.png ✅* +│ │ │ ├── yellow-arrow-player-k.png ✅* +│ │ │ └── yellow-arrow-player-s.png ✅* +│ │ ├── qdrill-logo-b-and-w.png ✅* +│ │ ├── qdrill-logo-colour.png ✅* +│ │ ├── qdrill-pill.png ✅* +│ │ └── quaffle.webp ✅* +│ └── robots.txt ✅* +├── svelte.config.js ✅ +├── tailwind.config.js ✅ +├── tsconfig.json ✅ +├── vercel.json ✅ +├── vite.config.js ✅ +└── vitest.config.js ✅ +``` + +Note files: +├── code-review.md +├── config-app-notes.md +├── diagram-notes.md +├── drill-notes.md +├── library-notes.md +├── modal-notes.md +├── practice-plan-notes.md +├── practice-plan-wizard-notes.md +├── service-notes.md +└── shared-components-notes.md +└── practice-plan-modal-notes.md +└── routes-base-notes.md +└── formations-notes.md +└── api-auth-notes.md +└── api-comments-notes.md +└── api-feedback-notes.md +└── api-poll-notes.md +└── api-votes-notes.md +└── api-tests-notes.md +└── feedback-page-notes.md +└── poll-page-notes.md + +## Code Review Summary + +The code review process is complete. + +- **Files Reviewed:** 224 +- **Lines of Code Reviewed:** ~19,703 (Svelte/JavaScript in src/, excluding mocks/tests/images) +- **Tickets Created:** 33 + +The findings and recommendations have been documented in individual ticket files located in the `tickets/` directory, summarized in `holistic-summary.md`, and linked from `tickets/index.md`. diff --git a/code-review/config-app-notes.md b/code-review/config-app-notes.md new file mode 100644 index 00000000..c3afd403 --- /dev/null +++ b/code-review/config-app-notes.md @@ -0,0 +1,65 @@ +# Configuration and Core App File Review Notes + +This file contains notes from the review of root configuration files (`svelte.config.js`, `tailwind.config.js`, `tsconfig.json`, `vercel.json`, `vite.config.js`, `vitest.config.js`) and core app files (`src/app.html`, `src/hooks.server.js`, `src/styles.css`). + +## Files Reviewed + +- `svelte.config.js` +- `tailwind.config.js` +- `tsconfig.json` +- `vercel.json` +- `vite.config.js` +- `vitest.config.js` +- `src/app.html` +- `src/hooks.server.js` +- `src/styles.css` + +## Notes + +**Overall:** Configurations are mostly standard for a SvelteKit project using Vercel, Tailwind, TypeScript, and Vitest. Core app files (`app.html`, `hooks.server.js`, `routes/styles.css`) are straightforward. + +**Key Findings & Recommendations:** + +1. **TypeScript Strictness (`tsconfig.json`):** + + - `noImplicitAny` is set to `false`. + - **Recommendation:** Enable `strict: true` in `compilerOptions` (or at least `noImplicitAny: true`) for significantly improved type safety and maintainability. This is a standard practice in professional TypeScript projects. + +2. **Color Theme Inconsistency (`tailwind.config.js` vs `src/routes/styles.css`):** + + - `tailwind.config.js` defines theme colors (e.g., `theme-1: '#ff3e00'` - orange). + - `src/routes/styles.css` defines CSS variables for colors (e.g., `--color-theme-1: #3b82f6` - blue) and uses them for the body background and some specific classes (`.selected`). + - Drag-and-drop styles in `src/routes/styles.css` use hardcoded blue values (`#3b82f6`). + - **Recommendation:** Consolidate the color theme definitions. Either: + - Make the CSS variables in `styles.css` match the Tailwind theme colors and use the CSS variables consistently. + - Remove the theme-related CSS variables and rely _only_ on Tailwind utility classes (`bg-theme-1`, `text-theme-1`, etc.) generated from `tailwind.config.js`. This is often the preferred approach when using Tailwind. + - Refactor hardcoded colors in `styles.css` (like drag/drop styles) to use the chosen theme system (Tailwind classes or CSS variables). + +3. **Vercel Rewrites (`vercel.json`):** + + - Contains rewrites: `{ "source": "/api/drills/(.*)", "destination": "/api/drills" }` and similar for practice plans. + - **Question:** Are these rewrites still necessary? SvelteKit's standard filesystem routing (`src/routes/api/drills/[id]/+server.js`) usually handles parameterized routes automatically. + - **Recommendation:** Investigate if these rewrites can be removed. They might be redundant or could potentially interfere with SvelteKit's expected routing behavior. + +4. **Error Handling Cleanup (`src/hooks.server.js`):** + + - The `handleError` hook calls `cleanup()` from `$lib/server/db` on _any_ uncaught error. + - **Question:** What exactly does `db.cleanup()` do? Does it handle transaction rollbacks, release connections, or something else? Is it safe and appropriate to call unconditionally on all errors? + - **Recommendation:** Review the implementation of `db.cleanup()` to ensure it's robust, doesn't hide the original error (e.g., if cleanup itself fails), and doesn't have unintended side effects when called outside specific contexts (like after a database operation). + +5. **Vitest Globals (`vitest.config.js`):** + + - `globals: true` is configured. + - **Recommendation (Minor):** Consider setting `globals: false` and using explicit imports (`describe`, `it`, `expect`, etc.) in test files. This makes dependencies clearer and avoids potential global namespace conflicts. + +6. **TS Module/Target (`tsconfig.json`):** + - Uses `target: "es6"` and `module: "commonjs"`. + - **Recommendation (Minor):** Consider updating to more recent ECMAScript targets/modules (e.g., `es2020`, `esnext`) if compatible with the runtime environment (Node version on Vercel) and dependencies, although `commonjs` is often necessary for Node. + +**Minor Points:** + +- `svelte.config.js`: Minimal, uses Vercel adapter, prerenders root. Standard. +- `vite.config.js`: Includes React deps for Excalidraw. Defines useful aliases. Standard. +- `vitest.config.js`: Mocks `$app/*` modules correctly for testing. Coverage setup looks reasonable. +- `src/app.html`: Standard SvelteKit HTML shell. +- `src/routes/styles.css`: Defines some utility classes (`.visually-hidden`) and drag/drop styles (see point 2 regarding hardcoded colors). diff --git a/code-review/diagram-notes.md b/code-review/diagram-notes.md new file mode 100644 index 00000000..83ba1a42 --- /dev/null +++ b/code-review/diagram-notes.md @@ -0,0 +1,22 @@ +## Diagrams Feature Review + +This section reviews the code related to creating and managing diagrams using Excalidraw. + +**Files Reviewed:** + +- `src/components/ExcalidrawWrapper.svelte` + - **Notes:** Wraps the `@excalidraw/excalidraw` React component for Svelte. Handles initialization, data loading (from prop or template), fullscreen editing mode, and saving scene data. Dynamically imports React/Excalidraw. Includes logic to programmatically generate initial elements for templates ('blank', 'halfCourt', 'fullCourt') and a sidebar library of draggable icons (players, hoops, balls, cone). Converts image assets to base64 Data URLs for embedding. + - **Potential Issues:** + - **Major: React Dependency:** Pulling `react`, `react-dom`, and `@excalidraw/excalidraw` into a Svelte project adds significant complexity, bundle size, and potential performance overhead. Managing the React component lifecycle manually (`ReactDOM.createRoot`, `root.render`, `root.unmount`) within Svelte hooks is non-standard. + - **High Complexity:** The component is very large (>1000 lines) and handles multiple distinct concerns: Excalidraw rendering, data serialization/deserialization, template generation, image fetching/conversion, fullscreen modal logic, sidebar element creation, view/zoom management. Consider breaking down responsibilities. + - **State Synchronization:** Managing state between the inline preview instance and the separate fullscreen modal instance (copying `elements`, `appState`, `files`) appears complex and potentially fragile. + - **Image Data URLs:** Fetching images and converting them to Base64 Data URLs (`fetchImageAsDataURL`) happens on component initialization and template generation. This increases initial load time and significantly bloats the saved scene data JSON. While potentially necessary for Excalidraw's image handling, it's a notable drawback. Explore if Excalidraw offers alternative image handling (e.g., referencing URLs directly if persistence isn't needed within the Excalidraw blob itself, or using the file cache more effectively). + - **Hardcoded Layouts:** Template element positions (`addHalfCourtElements`, `addFullCourtElements`) and sidebar element positions (`addSidebarElements`) are hardcoded. This makes layout adjustments cumbersome. Consider defining layouts in configuration objects. + - **Fullscreen Implementation:** Creates an entirely new Excalidraw instance for fullscreen mode (`createFullscreenComponent`) rather than potentially reusing or simply restyling the existing one. This doubles the resources needed when fullscreen is active. + - **Asynchronous Timing:** Relies on `tick()` and `setTimeout` in several places (especially around fullscreen transitions and initial zoom) which can sometimes indicate tricky timing requirements or race conditions. + - **Guide Rectangle Logic:** The presence of `fixGuideRectanglePosition` suggests potential issues with elements drifting or the guide rectangle not staying at (0,0), requiring a corrective mechanism. + - Minor: Uses `window.process` polyfill, which is standard but worth noting. Error handling uses `console.error`/`console.warn`. + +**Files Pending Review:** + +_(Add more files as review progresses)_ diff --git a/code-review/drill-notes.md b/code-review/drill-notes.md new file mode 100644 index 00000000..86c38a1f --- /dev/null +++ b/code-review/drill-notes.md @@ -0,0 +1,177 @@ +## Drills Feature Review + +This section reviews the code related to browsing, creating, viewing, and editing drills. + +**Files Reviewed:** + +- `src/routes/drills/+page.svelte` ✅ + - **Notes:** Good use of SvelteKit features (load, stores, goto). Handles filtering, sorting, pagination. + - **Potential Issues:** + - `{@html}` used for `brief_description` - requires verification of backend sanitization to prevent XSS. + - `buttonStates` logic for cart feedback uses `setTimeout`, which is less declarative/robust than deriving state directly. + - Component handles many responsibilities; consider extracting `DrillCard`. +- `src/routes/drills/+page.server.js` ✅ + - **Notes:** Correctly uses `load` function, parses URL params, uses service layer (`drillService`), fetches data in parallel (`Promise.all`). Good error handling structure. + - **Potential Issues:** Ensure filter parsing is robust; consider caching for `filter-options` if appropriate. +- `src/routes/drills/[id]/+page.svelte` ✅ + - **Notes:** Displays drill details, variations, diagrams, comments. Includes logic for managing variants (modal, search), editing diagrams, adding to cart, deleting. + - **Potential Issues:** + - `{@html}` used for `detailed_description` - requires backend sanitization verification. + - High component complexity; many responsibilities (display, variant management, diagram interaction, API calls). + - Local `writable` store (`drill`) duplicates server data; consider using reactive `data` prop directly more often. + - Direct `fetch` calls within component; should be abstracted to service/store. + - Uses `alert()` instead of `svelte-toast` for cart feedback. + - Presence of `console.log` statements. + - Variant management modal is a prime candidate for component extraction. +- `src/routes/drills/[id]/+page.server.js` ✅ + - **Notes:** Simple load function fetching data via internal API (`/api/drills/[id]`). Basic error handling. + - **Potential Issues:** Could return more specific HTTP error status codes. +- `src/routes/drills/create/+page.svelte` ✅ + - **Notes:** Simple wrapper, passes data from load function to `DrillForm`. + - **Potential Issues:** None noted. +- `src/routes/drills/create/+page.server.js` ✅ + - **Notes:** Clean load function using service layer to fetch data for the form (skills, drill names). Good error handling. + - **Potential Issues:** None noted. +- `src/routes/drills/DrillForm.svelte` ✅ + - **Notes:** Shared component for create/edit. Handles many input types, dynamic lists (diagrams, images), validation, TinyMCE integration, basic auth flow for saving. + - **Potential Issues:** + - **Major:** Direct `fetch` calls within the component for submitting the form and adding skills. Needs abstraction (service/store/form action). + - High component complexity/size. Should be broken down into smaller, focused components (e.g., `SkillInput`, `DiagramList`, `ImageUpload`). + - Imperative child component interaction (`diagramRefs.saveDiagram()`). Event dispatching or binding might be cleaner. + - Auth flow uses `sessionStorage`, which can be brittle. + - Verbose state management (individual `writable` per field). + - Uses browser `confirm()` instead of integrated modals. + - Presence of `console.log`. +- `src/routes/drills/[id]/edit/+page.svelte` ✅ + - **Notes:** Clean wrapper component. Uses reactive destructuring. Passes data to `DrillForm`. Basic loading/error display. + - **Potential Issues:** None noted (relies on `DrillForm` for main functionality/issues). +- `src/routes/drills/[id]/edit/+page.server.js` ✅ + - **Notes:** Excellent load function. Fetches drill, skills, names. Includes crucial server-side authorization check (`canUserEdit`) and good error handling (400, 403, 404, 500). + - **Potential Issues:** None noted. +- `src/routes/api/drills/+server.js` ✅ + - **Notes:** Handles GET (list w/ filters, sort, pagination) and POST (create) for drills. Uses `drillService`. GET parsing is robust. POST passes data and user ID to service. + - **Potential Issues:** + - **Major:** Contains non-functional `DELETE` handler (tries to use `params.id`). DELETE logic must be in `[id]/+server.js`. + - **Unconventional:** Contains `PUT` handler. While functional (gets ID from body), PUT is typically on the `[id]/+server.js` route. + - Minor duplication of filter parsing logic with `drills/+page.server.js`. +- `src/routes/api/drills/[id]/+server.js` ✅ + - **Notes:** Correctly handles GET (one), PUT (update), DELETE for specific drill ID. Uses `drillService`. Includes visibility/auth checks. GET handler enriches data (variant names, parent name). Good error handling structure (constants, helper function). + - **Potential Issues:** + - **Cross-Domain Update:** PUT handler directly updates the `votes` table via `db.query`. This should ideally be handled via a `voteService` or within the `drillService` to maintain separation of concerns. + - DELETE handler has a slightly awkward `dev` mode check that bypasses service-level auth; could potentially be handled via configuration/service behavior. + - Minor: GET handler fetches user names with direct `db.query`; could be abstracted to `userService` or `drillService`. +- `src/routes/api/drills/filter-options/+server.js` ✅ + - **Notes:** Provides distinct values/ranges for filter fields. Uses parallel SQL queries. Attempts normalization in SQL. + - **Potential Issues:** + - **Major:** Creates its own DB client instead of using shared `$lib/server/db.js`. Needs refactoring. + - **Major:** Contains complex data fetching logic directly in the API handler. Should be abstracted to `drillService.getFilterOptions()`. + - **Performance:** Lacks caching. Filter options are prime candidates for caching (in-memory or external) to reduce DB load. + - Data inconsistency suggested by `processResults` function attempting to parse JSON strings from results. + - `suggestedLengths` range is hardcoded; might need to be dynamic. + +**Files Pending Review:** + +- `src/lib/server/services/drillService.js` ✅ + - **Notes:** Extends `BaseEntityService`, enabling standard permissions. Defines drill-specific columns, allowed columns for filtering/sorting, and column types (especially `json` for `diagrams`, `array` for skills, positions, etc.). Overrides `createDrill` and `updateDrill` to handle skill count updates (`updateSkills`, `updateSkillCounts`) within transactions. Overrides `deleteDrill` to enforce stricter deletion permissions (only creator). Implements complex filtering in `getFilteredDrills`, handling array overlaps (`&&`), range queries, boolean checks (using indexed conditions for `hasVideo`, `hasDiagrams`, `hasImages`), text search (`ILIKE`), and visibility rules. Includes methods for managing variations (`getDrillWithVariations`, `createVariation`, `setAsPrimaryVariant` - complex swap logic). Provides `searchDrills` (using base search) and `getDrillNames`. Implements `toggleUpvote` using transactions to manage votes in the `votes` table. Includes `setVariant` for associating/dissociating variations. Contains `normalizeDrillData` helper for data consistency (handling arrays, JSON stringification for diagrams, number fields). Provides `associateDrill` to assign ownership to anonymously created drills. Uses `_addVariationCounts` helper to efficiently add counts to drill lists. + - **Potential Issues:** + - **`getFilteredDrills` Complexity:** This method is very complex, manually building a large SQL query with many conditional filters. This is powerful but prone to errors and hard to maintain. The logic for filtering ranges (`number_of_people`, `suggested_length`) seems particularly complex and might need verification. Using a query builder library could simplify this. + - **Filtering Performance:** While some indexed conditions are used (`hasVideo`, etc.), the array overlap (`&&`) and `ILIKE` operators might not be optimally performant on large datasets without specific GIN/GIST indexes on array columns and potentially trigram indexes for text search. Performance testing is recommended. The complexity calculation uses `COUNT(*)`, which can be slow. + - **Skill Update Logic:** `updateSkills` uses `ON CONFLICT DO UPDATE` with a `CASE` statement checking `NOT EXISTS` in a subquery to conditionally increment `drills_used_in`. This is complex SQL logic; ensure it correctly handles all edge cases (e.g., adding/removing skills in the same update). + - **`setAsPrimaryVariant` Complexity:** The logic using a temporary negative ID swap is clever but highly unconventional and potentially fragile if IDs could clash or if negative IDs have other meanings. A more standard approach might involve copying data between records. + - **Data Normalization:** `normalizeDrillData` explicitly stringifies diagrams (`JSON.stringify`). Ensure this is the desired format for storage and retrieval. It also normalizes array items to lowercase strings, which might be lossy if case sensitivity is ever needed. + - **Direct DB Calls:** Still contains direct `db.query` calls for variations (`getDrillWithVariations`), skills (`updateSkills`, `updateSkillCounts`), votes (`toggleUpvote`), and names (`getDrillNames`), bypassing potential base service abstractions. + - **Error Handling:** Uses `throw new Error('Message')`, relying on API layers to interpret messages. Custom error types would be more robust. + - **`associateDrill`:** Uses `update` from base class, assumes base class correctly handles permissions/updates for this specific case. +- `src/lib/stores/drillsStore.js` ✅ + - **Notes:** Centralizes state for the drills list (data, pagination, loading, filters, sorting). Uses `writable` and `derived`. Includes `initializeDrills` and `resetDrillFilters` helpers. Well-organized. + - **Potential Issues:** + - Uses separate `writable` stores for each filter category; could consolidate into a single filter object store if complexity grows. + - Relies on constants and other stores; ensure dependency management is clear. + - `initializeDrills` uses `console.warn` for invalid data. + +## Drill API Routes Review + +- `src/routes/api/drills/[id]/variations/+server.js` ✅ + - **Notes:** Handles GET (list variations including parent/siblings) and POST (create variation). Uses `drillService` (`getById`, `getDrillWithVariations`, `createVariation`). GET logic intelligently fetches context (parent, siblings) depending on whether the requested `[id]` is a parent or a child. POST requires logged-in user and validates `name`. Includes a shared error helper. + - **Potential Issues:** + - **GET Complexity:** The logic in GET to handle both parent/child cases and reorder results is complex and could be hard to follow or debug. + - **Error Handling:** Relies on specific error message strings caught from the service (e.g., 'Parent drill not found'). Using custom error classes propagated from the service would be more robust. + - **Authorization (GET):** The GET handler doesn't perform explicit authorization checks itself; it relies on the underlying `drillService` methods (`getById`, `getDrillWithVariations`) to enforce visibility rules based on the user (or lack thereof). This implicit reliance should be verified. POST correctly requires a session user ID. +- `src/routes/api/drills/[id]/associate/+server.js` ✅ + - **Notes:** Handles POST requests to assign ownership (`user_id`) of a presumably anonymously created drill (`[id]`) to the currently authenticated user. Correctly checks for an active session and user ID (returns 401 otherwise). Uses `drillService.associateDrill`. Handles specific error cases gracefully (invalid ID 400, not found 404, already owned 200 - idempotent). + - **Potential Issues:** Seems straightforward and uses the service layer appropriately. Authorization is correctly implemented. +- `src/routes/api/drills/[id]/set-variant/+server.js` ✅ + - **Notes:** Handles PUT requests to establish or remove the parent-child relationship between drills (setting `parent_drill_id` on the drill `[id]`). Takes `parentDrillId` (can be null) from the request body. Uses `drillService.setVariant` to encapsulate the complex logic. Validates the incoming `drillId`. Catches specific error messages from the service (e.g., 'Drill not found', 'Cannot make a parent drill into a variant') and maps them to appropriate HTTP status codes (404, 400). + - **Potential Issues:** + - **Authorization:** No explicit authorization check within the handler. Relies entirely on `drillService.setVariant` to ensure the user making the request has permission to modify the drill relationships (e.g., is the owner). This delegation is acceptable but needs to be verified in the service implementation. +- `src/routes/api/drills/[id]/upvote/+server.js` ✅ + - **Notes:** Handles POST requests for a logged-in user to toggle their upvote status on a specific drill `[id]`. Checks for session and user ID (returns 401 if none). Validates `drillId`. Uses `drillService.toggleUpvote`, which presumably handles the transactional logic of updating both the drill's `upvotes` count and the `votes` table. Returns the updated `upvotes` count and the user's current voting status (`hasVoted`). Uses SvelteKit's `error` helper for 400 (Invalid ID) and 404 (Not Found) errors based on service feedback. + - **Potential Issues:** Clear implementation relying on the service layer. Authorization check is present. +- `src/routes/api/drills/search/+server.js` ✅ + - **Notes:** Handles GET requests to search for drills by name or retrieve a list of drill names (if the `query` parameter is empty). Parses URL parameters for `query`, `page`, `limit`, and `includePagination`. Uses `drillService.searchDrills` (returning only `id`, `name`) when a query exists and `drillService.getDrillNames` otherwise. Optionally includes pagination metadata in the response. + - **Potential Issues:** + - **Mixed Responsibilities:** The endpoint serves two slightly different purposes (search vs. get-all-names) based on the presence of the `query` param. While functional, dedicated endpoints might offer better clarity. + - **Arbitrary Limit:** When no query is provided, it defaults to fetching names via `drillService.getDrillNames` but then slices the result to 50 unless a `limit` is specified. This fallback limit seems arbitrary and potentially inconsistent with `getDrillNames` internal logic. + - **Authorization:** No explicit authorization checks. Relies on the underlying service methods (`searchDrills`, `getDrillNames`) to handle visibility filtering based on user context (if any). +- `src/routes/api/drills/names/+server.js` ✅ + - **Notes:** Handles GET requests specifically to retrieve a list of drill IDs and names, intended for populating dropdowns or similar UI elements. Retrieves the user ID from the session and passes it to `drillService.getFilteredDrills` to ensure only visible drills are returned. Configures the service call to sort by name and select only `id` and `name`. + - **Potential Issues:** + - **Hardcoded Limit:** Uses a hardcoded `limit: 1000` in the service call. This is problematic for scalability; if the number of drills exceeds 1000, the list will be incomplete. This endpoint should implement proper pagination or be used only in contexts where fetching _all_ names is truly necessary and feasible. + - **Overlap with `/search`:** Functionality significantly overlaps with `search/+server.js` when its `query` parameter is empty. The primary difference is this endpoint explicitly passes the `userId` for filtering, whereas `/search` relies implicitly on the service. Consider consolidating or better differentiating the use cases. +- `src/routes/api/drills/associate/+server.js` ✅ + - **Notes:** This route appears identical in function to `src/routes/api/drills/[id]/associate/+server.js`. It handles POST requests to assign ownership of an anonymous drill to the authenticated user. However, it expects the `drillId` in the _request body_ instead of the URL parameters. + - **Potential Issues:** + - **Redundancy:** Functionally duplicates the `[id]/associate` route. One should likely be removed to avoid confusion. Using the ID in the URL (`[id]/associate`) is more conventional REST practice for acting on a specific resource. + - **Authorization:** Correctly uses `authGuard` to ensure user is logged in. + - **Error Handling:** Handles not found and already owned errors. +- `src/routes/api/drills/bulk-upload/+server.js` ✅ + - **Notes:** Handles POST requests containing a CSV file (`multipart/form-data`) for parsing and _validating_ drill data. Uses `csv-parse` for parsing and `Yup` for validation against a detailed `drillSchema`. Maps numeric codes from the CSV (skill level, complexity) to string representations. Filters skills/positions/types against predefined lists. Adds user ID, visibility, and `is_editable_by_others` flags. Returns a summary (total, valid, errors) and the full list of parsed/validated drill objects, including row numbers and error messages for invalid ones. + - **Potential Issues:** + - **Validation Only:** This endpoint _only validates_ the CSV data; it does **not** actually insert the drills into the database. A separate step or endpoint (like `import/+server.js`) would be needed to persist the validated drills. This is a critical distinction. + - **Hardcoded Mappings:** Uses hardcoded maps (`skillLevelMap`, `complexityMap`, `drillTypeOptions`). Changes to these require code modification. + - **Validation Library:** Uses `Yup` for validation, which is a valid choice but introduces another dependency. SvelteKit's built-in form actions and validation could potentially be used, though `Yup` offers powerful schema definition. + - **Error Reporting:** Returns validation errors associated with each drill object. The frontend needs to handle displaying these clearly. + - **Authorization:** Correctly uses `authGuard`. +- `src/routes/api/drills/import/+server.js` ✅ + - **Notes:** Handles POST requests with a JSON payload containing an array of `drills` to be inserted into the database. Assigns the authenticated user's ID as `created_by`, uses a provided `visibility`, and sets `is_editable_by_others` to `false`. Generates a unique `upload_source` identifier using the provided `fileName` and a UUID. Performs the insertions within a database transaction (BEGIN/COMMIT/ROLLBACK). Handles diagram data (ensuring it's an array, stringifying each object). Uses `authGuard`. + - **Potential Issues:** + - **New DB Client:** Creates its own `pg.Pool` instead of using the shared `$lib/server/db.js` module. This bypasses the shared pool configuration and connection management, which is highly undesirable. Should be refactored to use `db.query` or `db.getClient`. + - **Data Validation:** Assumes the incoming `drills` array in the JSON payload has already been validated (e.g., by the `bulk-upload` endpoint). It performs minimal checks (is array, not empty) but directly attempts insertion. Errors during insertion (e.g., constraint violations) will cause the transaction to roll back. + - **Diagram Stringification:** Explicitly stringifies each diagram object before insertion. This matches the expectation noted in `drillService` review but reinforces the potential issues with storing stringified JSON. + - **Hardcoded Defaults:** Sets `is_editable_by_others` to `false`. Might need flexibility. +- `src/routes/api/drills/migrate-diagrams/+server.js` ✅ + - **Notes:** One-off utility endpoint (POST) to migrate existing Fabric.js diagrams stored in the `drills` table to the Excalidraw format using the `fabricToExcalidraw` utility. Fetches all drills with non-empty diagrams, iterates through them, converts diagrams (skipping nulls and already converted ones), and updates the `diagrams` column in the database with the stringified result. Runs all updates in parallel (`Promise.all`). + - **Potential Issues:** + - **New DB Client:** Creates its own `pg` client instance instead of using the shared `$lib/server/db.js`. Should be refactored. + - **One-Time Use:** This is clearly intended for a single data migration. Should be documented as such, secured (e.g., admin-only or environment-variable protected, currently unprotected), and potentially removed after the migration is successfully completed. + - **Error Handling:** Basic error handling; if one update fails, `Promise.all` rejects, but it doesn't report _which_ drill failed. + - **Scalability:** Fetches _all_ drills with diagrams at once. Could be memory-intensive for very large datasets. Batching might be necessary. +- `src/routes/api/drills/test-migration/+server.js` ✅ + - **Notes:** Development-only (checks `dev` environment variable) POST endpoint for testing the `fabricToExcalidraw` conversion utility. Takes a single `diagram` object in the request body and returns the converted Excalidraw object as JSON. + - **Potential Issues:** Useful utility for development/debugging the migration function. Protected by `dev` flag, which is appropriate. Should not be enabled in production. + +## Drill Page Components Review + +- `src/routes/drills/bulk-upload/+page.server.js` ✅ + + - **Notes:** Simple `load` function that fetches all drills (`/api/drills?all=true`). Includes basic error handling. + - **Potential Issues:** + - **Unused Data?** Fetches _all_ existing drills. It's unclear if/how this data is actually used by the `+page.svelte` component. The component seems focused on _new_ drills from the CSV, not existing ones. This fetch might be unnecessary and potentially slow. + - **API Endpoint:** Relies on the `/api/drills` GET endpoint having an `?all=true` parameter to bypass pagination. Ensure this API behavior is intended and efficient. + - **Error Handling:** Returns a generic `{ status: 500, error: 'Internal Server Error' }` which isn't ideal for the page; SvelteKit's `error()` helper might be better for displaying a proper error page. + +- `src/routes/drills/bulk-upload/+page.svelte` ✅ + - **Notes:** Complex component facilitating a multi-stage CSV upload process: 1) Upload CSV, 2) Set visibility, 3) Call `/api/drills/bulk-upload` to parse/validate, 4) Display parsed drills (filtered by validity), 5) Allow inline editing/validation of parsed drills, 6) Allow adding/editing diagrams (`ExcalidrawWrapper`), 7) Call `/api/drills/import` to save valid drills to DB. Uses local `writable` stores for state management (`uploadedFile`, `isUploading`, `uploadSummary`, `parsedDrills`, `filterOption`, `visibility`). Includes client-side validation (`validateDrill`) that largely duplicates the Yup schema logic from the API route. Provides a CSV template download. + - **Potential Issues:** + - **High Complexity:** Manages a complex workflow with significant state, UI logic, validation, and multiple API interactions. Prone to bugs and difficult to maintain. + - **Duplicate Validation:** Client-side `validateDrill` function re-implements validation logic already present (using Yup) in the `/api/drills/bulk-upload` endpoint. This duplication is error-prone. The component should rely _solely_ on the API's validation results returned in the `parsedDrills.errors` array. + - **State Management:** Uses many individual `writable` stores. Could potentially be grouped into a single store or use SvelteKit form actions for better state handling, especially for the editing state. + - **Inline Editing:** Allows editing parsed data _after_ server-side validation. Saving an edited drill re-runs the client-side validation. This flow is confusing; ideally, edits would be saved and immediately re-validated via an API call, or edits would be disallowed after the initial validation. + - **Diagram Handling:** Integrates `ExcalidrawWrapper` for adding/editing diagrams _before_ the drills are imported. This adds complexity; perhaps diagrams should only be editable _after_ successful import via the standard drill edit page. + - **Unused `saveChanges`:** The `saveChanges` function seems redundant as edits are applied directly to the local `parsedDrills` store. + - **UI/UX:** The process of upload -> review -> edit -> import could potentially be streamlined. Error display relies on checking specific error strings. + - **Performance:** Handling/rendering/validating a large number of parsed drills entirely on the client-side could become slow. + +## Drill API Asset Routes Review + +- `src/routes/api/drill-assets/`: diff --git a/code-review/feedback-page-notes.md b/code-review/feedback-page-notes.md new file mode 100644 index 00000000..7d8ed184 --- /dev/null +++ b/code-review/feedback-page-notes.md @@ -0,0 +1,4 @@ +# Feedback Page Notes + +- `src/routes/feedback/+page.server.js`: +- `src/routes/feedback/+page.svelte`: diff --git a/code-review/formations-notes.md b/code-review/formations-notes.md new file mode 100644 index 00000000..0b7bf447 --- /dev/null +++ b/code-review/formations-notes.md @@ -0,0 +1,145 @@ +# Formations Code Review Notes + +## `src/lib/stores/formationsStore.js` + +- **State Organization:** Uses multiple individual `writable` stores. Consider grouping related state into a single store (holding an object or using a custom store) for better encapsulation, especially if inter-state logic grows. +- **Initialization:** `initializeFormations` includes good safety checks and defaults for data and pagination. +- **Derived Stores:** Lacks `derived` stores. These could simplify logic by automatically reacting to changes in base stores (e.g., deriving a filtered list based on `formations` and filter stores). +- **Type Safety:** No explicit types (JS file). Add JSDoc or convert to TypeScript to define data structures (like the `data` parameter in `initializeFormations`) and improve maintainability. +- **Loading State:** Includes an `isLoading` store, which is good practice. + +## `src/routes/api/formations/+server.js` + +- **GET Handler:** + - Well-structured parameter handling (pagination, sorting, filters). + - Uses `formationService` effectively. + - Consider the necessity of hardcoding `columns` in `paginationOptions`; it might limit future API flexibility. + - Investigate the comment regarding `locals` potentially being unavailable; ensure consistent session/user data access. +- **POST Handler:** + - Handles request body and user association correctly. + - Good practice: Explicitly removes `id` before calling `createFormation`. + - Consider replacing `console.log` with a proper logger or making it conditional. +- **PUT Handler:** + - Correctly uses `authGuard`. + - Implements sound authorization logic (checks ownership or `is_editable_by_others` flag). + - Notes the intentional bypass of authorization in dev mode (`!dev`). +- **General:** + - Consistent error handling and JSON responses. + - Good separation of concerns (API route vs. service logic). + +## `src/routes/api/formations/[id]/+server.js` + +- **GET Handler:** Fetches a formation by ID using `formationService.getById`. Returns 404 if not found, 500 on error. Basic error logging included. Consider adding `authGuard` if formations are not meant to be publicly accessible. +- **DELETE Handler:** Uses `authGuard` for authentication. Checks for formation existence and verifies user ownership (or `dev` mode) before deletion via `formationService.delete`. Returns 404, 403, or 500 based on outcomes. +- **Overall:** Code is straightforward and uses the service layer appropriately. Error handling is present but minimal. The `dev` mode bypass for deletion permissions should be reviewed for security implications in production environments. + +## `src/routes/api/formations/[id]/associate/+server.js` + +- **POST Handler:** Associates the formation (by ID) with the logged-in user (`session.user.id`). +- **Auth:** Checks for active session/user ID, returns 401 if missing. +- **Validation:** Parses `formationId` and returns 400 if not a valid number. +- **Service Call:** Uses `formationService.associateFormation(formationId, userId)`. +- **Error Handling:** Catches specific errors from the service: + - 'Formation not found': Returns 404. + - 'Formation already has an owner': Fetches the formation and returns 200 (consider if 409/403 is more suitable). + - Other errors: Returns 500. +- **Consideration:** Using `authGuard` could streamline auth checks. The 200 status on 'already owned' might need review based on intended client behavior. + +## `src/routes/api/formations/search/+server.js` + +- **GET Handler:** Handles formation search using URL parameters (`q`, `page`, `limit`, `sortBy`, `sortOrder`). +- **Parameters:** Extracts parameters from `url.searchParams` with defaults. +- **Empty Search:** Returns an empty result structure if `q` is missing. +- **Service:** Calls `formationService.searchFormations` for the search logic. +- **Auth:** No authentication; assumes public search. +- **Validation:** Basic parsing for pagination. Relies on the service layer for deeper validation of pagination/sorting parameters. +- **Error Handling:** Standard try/catch, logs errors, returns 500 response. + +## `src/routes/formations/+page.svelte` + +- **Data Flow:** Correctly uses `export let data` and reactive statements (`$:`) to initialize the `formationsStore` from server-side `load` data. Follows SvelteKit best practices. +- **State Management:** Effectively uses the centralized `formationsStore` for all relevant UI state (filters, search, pagination, sorting). Correct use of store subscriptions (`$`). +- **Filtering/Sorting/Pagination:** Implemented using a central `applyFiltersAndNavigate` function that updates URL parameters via `goto`. This correctly triggers server-side data reloading. Search input is debounced. +- **UI/UX:** Good feedback with loading (`$navigating`) and empty states. Clear controls for filtering, sorting, and pagination. Sort dropdown has click-outside closing logic. +- **Code Quality:** Well-structured, uses data passed from `load` for filter options. Comments clarify evolution (e.g., removal of client-side fetch). +- **Minor Points:** Sort dropdown logic could potentially be a component. Commented-out `created_by` display on cards. +- **Overall:** Solid implementation of a dynamic list page driven by server data and client-side navigation triggers. + +## `src/routes/formations/+page.server.js` + +- **Parameter Extraction:** Correctly parses relevant query parameters from `url` with appropriate defaults. +- **Filter Handling:** Builds `filters` object correctly, including parsing tags and removing empty values. +- **Service Integration:** Calls `formationService.getFilteredFormations` with structured options. +- **Data Return:** Returns data in the structure expected by the page component. +- **Error Handling:** Uses `try...catch` and returns default data on error. Consider using the `error` helper from `@sveltejs/kit` for more idiomatic error handling. +- **Filter Options:** Comments note that fetching dynamic filter options (e.g., all tags) is not implemented (potential TODO). +- **User Context/Auth:** Comments discuss potential `userId` filtering via `locals` but reasonably omit it for a public list. +- **Logging:** Uses `console.log` (consider replacing/controlling for production). +- **Column Selection:** Hardcoded `columns` list noted (optimization vs. flexibility). +- **Overall:** Functional server-side load implementation. Main improvements could be in error handling style and adding dynamic filter options. + +## `src/routes/formations/FormationForm.svelte` + +- **State Management:** Uses individual `writable` stores for form fields. Consider consolidating into a single object store or custom store for simplification. Correctly initializes from/reacts to the `formation` prop. +- \*\*Diagram Handling (`ExcalidrawWrapper`): + - Handles initialization (parsing JSON, default structure). + - Implements Add (with modal/templates), Delete, Move, Duplicate. + - Duplication logic includes deep copy, new IDs, and importantly, `groupId` mapping. + - Correctly uses component refs (`diagramRefs`) and `saveDiagram()` calls + `await tick()` before submit to capture latest diagram state. + - Uses `diagramKey` strategy for forcing list re-renders. +- **Rich Text Editor (TinyMCE):** Dynamically imported for performance, with textarea fallback. Standard config. +- \*\*Form Submission (`handleSubmit`): + - Basic validation implemented. + - Complex but functional anonymous user flow: prompts login (pre/post submit), uses `sessionStorage` to persist form data across login redirect, associates formation post-login. + - Correct API method (POST/PUT) and endpoint usage. + - Uses `svelte-toast` for feedback. +- **Tags:** Standard implementation. +- **Component Structure:** Very large component. Consider refactoring into smaller sub-components (e.g., `DiagramListManager`, `FormationMetadataFields`) for better maintainability. +- **Overall:** A complex but seemingly robust component handling many features. The anonymous user flow is intricate but provides good UX. Key area for improvement is potential component splitting. + +## `src/routes/formations/[id]/+page.svelte` + +- **Data Loading:** Correctly uses `export let data` and `$: formation = data.formation;` from the server `load` function. +- **Display:** Clearly renders all formation details. Uses `{@html}` for rich text and `ExcalidrawWrapper` (readonly) for diagrams. Handles stringified diagram JSON. +- **Actions:** Includes Back, Edit, Delete buttons. +- **Permissions:** Edit/Delete buttons are conditionally rendered based on user session, ownership (`created_by`), `is_editable_by_others` flag, and `dev` mode, correctly reflecting API authorization. +- **Delete Implementation:** `handleDelete` uses `fetch` for the DELETE API call with confirmation. Error feedback uses `alert` (could use `svelte-toast` for consistency). +- **Code Quality:** Clean structure, comments show removal of old client-side fetching. +- **Overall:** Well-implemented detail page following SvelteKit patterns, correctly handling data display, permissions, and actions. + +## `src/routes/formations/[id]/+page.server.js` + +- **ID Validation:** Correctly parses and validates the `id` from `params`, using the `error` helper for invalid IDs (400). +- **Service Call:** Uses `formationService.getById` to fetch data. +- **Not Found Handling:** Correctly uses the `error` helper to throw a 404 if the formation isn't found. +- **Error Handling:** Excellent use of SvelteKit's `error` helper in the `catch` block for idiomatic error page handling. +- **Data Return:** Returns the `formation` object as expected. +- **Overall:** Concise, robust, and idiomatic SvelteKit load function for a detail page. + +## `src/routes/formations/[id]/edit/+page.svelte` + +- **Purpose:** Svelte page for the formation editing interface. +- **Structure:** Acts as a wrapper, importing and rendering the reusable `FormationForm.svelte`. +- **Data:** Receives `formation` data via the `data` prop (from server load function) and passes it to `FormationForm`. +- **Reusability:** Leverages `FormationForm` for the main UI and logic, promoting DRY. +- **Metadata:** Uses `` to set a dynamic page title and static description. +- **Overall:** Clean, simple, and follows standard SvelteKit patterns for an edit page. Relies heavily on the `FormationForm` component for the actual editing interface and functionality. + +## `src/routes/formations/[id]/edit/+page.server.js` + +- **Data Fetching:** Fetches formation using `formationService.getById`. Handles 'Not Found' (404 error). +- **Authentication/Authorization:** + - Correctly gets user session/ID via `locals`. + - Implements authorization check: allows edit if `is_editable_by_others`, user is creator, or `created_by` is null (implicitly editable). + - Includes `dev` mode bypass. + - **Redirection:** Uses `throw redirect(303, ...)` back to the view page if the user is not authorized. Good UX. Includes an `?error=unauthorized` query param (currently unused by view page). +- **Error Handling:** Correctly re-throws redirects and uses the SvelteKit `error` helper for other errors (500). +- **Data Return:** Returns `formation` data if authorized. +- **Overall:** Solid load function for an edit page, properly handling data fetching, authorization checks with redirection, and errors. + +## `src/routes/formations/create/+page.svelte` + +- **Simplicity:** Very simple wrapper component. +- **Component Reuse:** Excellent reuse - imports and renders the shared `FormationForm.svelte` component without a `formation` prop, relying on the form's default state for creation. +- **Metadata:** Sets appropriate page `` tags for creation. +- **Overall:** Clean and efficient use of component composition for the create view. diff --git a/code-review/holistic-summary.md b/code-review/holistic-summary.md new file mode 100644 index 00000000..7cc11c84 --- /dev/null +++ b/code-review/holistic-summary.md @@ -0,0 +1,99 @@ +# Holistic Code Review Summary + +This document synthesizes the findings from the detailed code review notes, providing a high-level overview of architectural patterns, cross-cutting concerns, and key areas for improvement across the QDrill codebase. + +## Key Themes & Architectural Observations + +1. **State Management Complexity & Coupling:** + + - **Overly Complex Stores:** `sectionsStore` (>1000 lines managing nested plan structures) and `practicePlanStore` (mixing form state, API logic, filters, utils) are excessively complex and tightly coupled, violating single responsibility. `dragManager` is also very complex. + - **Wizard vs. Form State Duplication:** The Practice Plan Wizard (`wizardStore`, `wizardValidation`) largely duplicates state (especially `sections`) and logic (validation, submission flow) found in the main form (`practicePlanStore`, `sectionsStore`). This leads to confusion and maintenance overhead. + - **Store Dependencies:** Components are often tightly coupled to specific store implementations (e.g., shared practice plan components, modals importing store functions directly), hindering reusability (especially between the wizard and the main form). + - **Reactivity Issues:** Some areas show signs of fighting Svelte's reactivity (e.g., `TimelineSelectorModal` using `setTimeout` and manual updates), suggesting potential issues in state management patterns. + - **Unused Stores:** The previously unused `dragStore.js` file has been removed. The obsolete `feedbackList` export was also deleted from `feedbackStore`. + +2. **API Design, Implementation & Scalability:** + + - **Scalability Bottlenecks:** Several `GET` endpoints (`/api/drills`, `/api/practice-plans`) and corresponding `+page.server.js` load functions fetch _all_ data without pagination, server-side filtering, or sorting capabilities exposed via URL parameters. This is a major scalability concern. Client-side filtering/sorting (`/drills`, `/practice-plans` pages) will not perform well with larger datasets. + - **Inconsistent Error Handling:** Error handling varies between API routes (generic 500 vs. specific 40x codes). Many rely on matching specific error message strings from services, which is brittle. + - **Authorization Gaps:** Critical authorization check missing in `src/routes/practice-plans/[id]/edit/+page.server.js`. Use of `dev` mode to bypass permissions in API routes (`/api/practice-plans/[id]/`, `/api/formations/`) is risky. `authGuard` is used inconsistently. + - **REST Conventions:** Some non-standard practices (e.g., PUT/DELETE handlers in collection routes like `/api/drills/+server.js`). Redundant endpoints exist (e.g., `/api/drills/associate/` vs `/api/drills/[id]/associate/`). + - **Service Layer Bypass:** Some API routes create their own DB clients (`filter-options`, `import`, `migrate-diagrams`) or make direct DB calls, bypassing the service layer and shared connection pool. + - **Unused APIs:** Feedback-related API routes and server functions (`/api/feedback`, `lib/server/feedback.js`) appear unused. + +3. **Component Design & Reusability:** + + - **High Component Complexity:** Components like `ExcalidrawWrapper`, `PracticePlanForm`, and `FilterPanel` are very large and handle too many concerns, making them difficult to maintain. + - **Store Coupling:** As mentioned, direct imports of store functions prevent component reuse (e.g., practice plan item/section components cannot be easily shared between the main form and the wizard). + - **Potential XSS:** Use of `{@html}` in several places (`drills/[id]/+page.svelte`, `practice-plans/[id]/+page.svelte`) requires careful backend sanitization verification. + - **Accessibility (A11y):** Potential issues noted in modals (focus trapping, ARIA attributes) and custom controls (`ThreeStateCheckbox`). Needs systematic review. + - **React Dependency:** `ExcalidrawWrapper` introduces React dependencies and complex lifecycle management within Svelte. + +4. **Code Duplication:** + + - **Wizard vs. Form:** Significant logic duplication between the practice plan wizard and the main cart-based form (state management, validation, UI components for sections/timelines). + - **Validation:** Validation logic is duplicated between client-side (component/store) and server-side (API), and also between the wizard and the main form. + - **SQL Logic:** Complex `ON CONFLICT DO UPDATE` logic for skill counts is duplicated between `DrillService` and `SkillService`. + +5. **Database & Service Layer:** + + - **Base Service Limitations:** `BaseEntityService` provides a good foundation but has limited filtering capabilities (`getAll`), a rigid permission model, and basic search. Inheriting services often bypass it with direct DB calls or complex overrides. + - **Complex Service Logic:** `DrillService` (`getFilteredDrills`, variant logic) and `PracticePlanService` (nested structure handling, delete-then-insert update strategy) contain very complex logic. + - **Normalization Issues:** Inconsistent handling of JSON data (e.g., stringifying diagrams in `DrillService` vs. not in `FormationService`). + - **Hardcoded Admin Logic:** `UserService` uses a hardcoded email list for admin checks, which is insecure. Its `canUserPerformAction` duplicates permission logic from elsewhere. + - **Performance:** `getUserProfile` fetches excessive related data. Complex queries (`getFilteredDrills`) and search methods need performance review/indexing. + +6. **Authentication & Authorization:** + + - **Fragile Auth Flow:** `PracticePlanForm` uses `sessionStorage` to persist state across OAuth redirects, which is unreliable. + - **Inconsistent Checks:** Authorization checks are missing or inconsistent, as noted in API and page load functions. + +7. **Configuration & Environment:** + + - **TypeScript Strictness:** Key strict checks (`noImplicitAny`) are disabled in `tsconfig.json`. Enabling stricter checks is highly recommended. + - **Theme Inconsistency:** Discrepancy between Tailwind theme colors and CSS variable definitions (`tailwind.config.js` vs `src/routes/styles.css`). + - **Vercel Rewrites:** Potentially redundant rewrites in `vercel.json`. + +8. **Unused/Dead Code:** + - Significant amounts of potentially unused code identified: `Counter.svelte`, `feedback.js`, `loggerUtils.js`, portions of `feedbackStore` (prior to cleanup), `src/lib/vitals.js` (disabled). + +## Overarching Recommendations + +1. **Refactor State Management:** + + - **Unify Practice Plan State:** Eliminate the state duplication between the wizard and the main form. Ideally, have the wizard directly use the main `sectionsStore` (potentially refactored). + - **Simplify Stores:** Break down bloated stores (`practicePlanStore`, `sectionsStore`) into smaller, single-responsibility stores. Decouple stores from API calls and side effects (use Form Actions or dedicated API services). + - **Improve Component Coupling:** Pass data and action callbacks via props to shared components instead of having them import store functions directly. + +2. **Address Scalability:** + + - **Implement Server-Side Pagination/Filtering/Sorting:** Modify API collection endpoints (GET `/api/drills`, `/api/practice-plans`, etc.) and corresponding `+page.server.js` load functions to accept and handle pagination, filtering, and sorting parameters via the URL. Remove client-side filtering/sorting for lists. + - **Optimize Database Queries:** Review complex queries (`getFilteredDrills`, `getUserProfile`) for performance. Ensure appropriate database indexes (e.g., GIN indexes for array columns, trigram indexes for `ILIKE`). + +3. **Strengthen API Design & Authorization:** + + - **Consistent Error Handling:** Implement a consistent error handling strategy across all API endpoints (e.g., using custom error classes/codes from services). + - **Enforce Authorization:** Systematically review and add missing authorization checks in `+page.server.js` load functions (especially for edit pages) and API routes. Remove `dev` mode permission bypasses or implement proper role-based access. Use `authGuard` consistently where needed. + - **Adhere to REST Conventions:** Refactor API routes to follow standard REST patterns (e.g., PUT/DELETE on `/[id]` routes). Remove redundant endpoints. + - **Eliminate Service Bypass:** Refactor API routes to always use the service layer and the shared DB connection pool. + +4. **Reduce Code Duplication:** + + - **Centralize Validation:** Create a shared validation library/schema (e.g., using Zod/Yup) usable by both client and server, and by both the wizard and the main form. + - **Abstract Common Logic:** Refactor duplicated SQL or complex business logic into shared helpers or service methods. + +5. **Improve Component Structure & Robustness:** + + - **Refactor Large Components:** Break down monolithic components (`ExcalidrawWrapper`, `PracticePlanForm`, `FilterPanel`) into smaller, focused sub-components. + - **Address `{@html}` Usage:** Ensure robust server-side sanitization for all user-provided content rendered with `{@html}`. + - **Review Accessibility:** Conduct a thorough accessibility review, particularly for modals and custom form controls. + +6. **Enhance Configuration & Environment:** + + - **Enable TypeScript Strictness:** Gradually enable stricter TypeScript checks in `tsconfig.json`. + - **Consolidate Theme:** Unify color definitions between Tailwind config and CSS variables. + +7. **Remove Dead Code:** + - Identify and remove unused components, stores, services, and utility functions after verification. + +By addressing these core themes, particularly state management consolidation, API scalability, authorization enforcement, and code duplication, the codebase can be significantly improved in terms of maintainability, scalability, security, and overall robustness. diff --git a/code-review/library-notes.md b/code-review/library-notes.md new file mode 100644 index 00000000..a0e61b4a --- /dev/null +++ b/code-review/library-notes.md @@ -0,0 +1,147 @@ +# Library Code Review Notes + +This file contains notes on the review of the `src/lib` directory, evaluating code structure, potential issues, and usage. + +## `src/lib/__mocks__` + +- `environment.js`, `navigation.js`, `stores.js` + - **Notes:** These files provide mock implementations for SvelteKit's environment variables (`$app/environment`), navigation functions (`$app/navigation`), and stores (`$app/stores`). They export simple constants or no-op functions, suitable for isolating components or functions during unit testing (e.g., with Vitest). + - **Potential Issues:** None apparent. They serve their purpose as simple mocks for testing environments where the actual SvelteKit modules aren't available or needed. Standard practice for unit testing. Marked as reviewed (`✅*`) in `code-review.md`. + +## `src/lib/constants/skills.js` + +- **Notes:** Exports a single constant array `PREDEFINED_SKILLS` containing a list of strings representing various skills relevant to the application domain (presumably sports drills). +- **Potential Issues:** + - **Hardcoded:** The list is hardcoded. If these skills need to be managed (added/edited/deleted) dynamically, they should be moved to the database and fetched via an API/service. However, for a stable, predefined list, this approach is acceptable. + - **Usage:** Checked previously via `grep_search`. It's used in the Skills API (`api/skills/+server.js`), Drill Form (`routes/drills/DrillForm.svelte`), bulk upload (`api/drills/bulk-upload/+server.js`), and initializing the `drillsStore`. Seems appropriately used. + +## `src/lib/constants.js` + +- **Notes:** Exports a single constant object `FILTER_STATES` with string values representing the three possible states for filter controls (`neutral`, `required`, `excluded`). +- **Potential Issues:** + - **Usage:** Checked previously via `grep_search`. It's widely used in components dealing with filtering logic, such as `FilterPanel.svelte`, `ThreeStateCheckbox.svelte`, `practicePlanStore.js`, `drillsStore.js`, and various page components (`practice-plans/+page.svelte`). This ensures consistency in filter state representation. Seems appropriate. + +## `src/lib/server/db.js` + +- **Notes:** Manages the application's PostgreSQL connection pool using the `pg` library. Implements a singleton pattern for the pool (`getPool`). Exports an async `query` function for executing parameterized queries, which handles client connection acquisition and release. Also exports `getClient` for obtaining a client instance directly (necessary for manual transaction management) and `cleanup` (currently a no-op, intended for request-specific resource cleanup) and `end` (for gracefully closing the pool). Includes basic error handling for idle clients. Connection parameters are taken from `process.env.POSTGRES_URL`. Sets pool limits (`max: 10`). +- **Potential Issues:** + - **`end()` Usage:** The `end()` function, intended for graceful shutdown, appears **unused**. While Node.js might terminate connections on exit, explicitly calling `end()` during application shutdown (e.g., via process signal handlers) is best practice to ensure the pool drains correctly. + - **`cleanup()` Purpose:** The `cleanup()` function is a no-op. Its intended purpose isn't clear from the code or comments ("request-specific cleanup"). If it's meant to be used (e.g., in `hooks.server.js`), it should either have functionality or be removed. Currently, its usage in `hooks.server.js` does nothing. + - **Error Handling:** Pool error handling (`pool.on('error', ...)`) logs the error but sets `pool = null`. While this might trigger pool recreation on the next `getPool()` call, it could be more robust (e.g., attempting reconnection or implementing circuit breaking). Query error handling in the `query` function logs the error and re-throws, allowing callers to handle it, which is good. + - **Configuration:** Pool settings (`max`, `idleTimeoutMillis`, etc.) are hardcoded. Making these configurable via environment variables might be beneficial. SSL is configured with `rejectUnauthorized: false`, which might be necessary for certain hosting providers (like Vercel Postgres) but implies less strict security; ensure this is the intended configuration. + +## `src/lib/server/auth.js` + +- **Notes:** Configures and exports Auth.js (formerly NextAuth.js) functionality for SvelteKit using `@auth/sveltekit`. Sets up the Google OAuth provider using client ID/secret from environment variables. Configures the Postgres adapter (`@auth/pg-adapter`) to interact with the database, passing it the custom `query` function from `./db.js`. Defines a `session` callback to include the user's database `id` in the session object, making it available in `locals.session`. Sets `trustHost: true` (common for SvelteKit deployments) and `secret` from environment variables. Includes a placeholder `signIn` callback. Enables debug mode. +- **Potential Issues:** + - **Callbacks:** The `signIn` callback currently just returns `true`. Depending on requirements, this could be used for custom logic like restricting sign-ins based on email domain or creating associated user profiles in other tables on first sign-in. + - **Adapter Query Mapping:** The adapter configuration manually maps the `db.js` `query` function's return shape (`{ rows, rowCount }`) to what the adapter expects. Ensure this mapping remains compatible if the `db.js` or `@auth/pg-adapter` changes. + - **Debug Mode:** `debug: true` is enabled, which is useful in development but should generally be **disabled in production** to avoid leaking sensitive information in logs. + +## `src/lib/server/authGuard.js` + +- **Notes:** Provides a higher-order function `authGuard` intended to wrap SvelteKit `load` functions or API route handlers. It retrieves the user session from `event.locals.getSession()`. If no session or user exists, it throws a 401 Unauthorized error using SvelteKit's `error` helper. Otherwise, it calls the original handler function (`handler(event)`). +- **Potential Issues:** + - **Usage:** Checked previously via `grep_search`. It's used correctly in various API routes and some server `load` functions to protect resources that require authentication. However, it was noted as crucially **missing** from `src/routes/practice-plans/[id]/edit/+page.server.js`. + - **Granularity:** It performs a simple check for _any_ logged-in user. It doesn't handle role-based or permission-based authorization checks; such logic would need to be implemented within the wrapped handler or a separate authorization mechanism. + +## `src/lib/server/feedback.js` + +- **Notes:** Defines server-side functions for managing user feedback: `saveFeedback` (inserts into `feedback` table), `getAllFeedback` (retrieves all feedback), `upvoteFeedback` (increments upvotes), and `deleteFeedback` (deletes by ID). Uses the `query` function from `./db.js` for database interactions. +- **Potential Issues:** + - **Unused Code:** Based on previous `grep_search`, this entire module appears **unused**. The corresponding API routes or components that would call these functions seem to be missing or non-functional. This represents dead code that should likely be **removed** unless the feedback feature is planned for implementation. + - **Authorization:** None of the functions implement any authorization checks. `getAllFeedback` would return all feedback to anyone, and `upvoteFeedback`/`deleteFeedback` could be called by anyone if exposed via an API without protection. If this were to be used, authorization (e.g., admin-only for delete/view all, user-specific for upvote) would be essential. + - **Error Handling:** Basic error handling relies on the `query` function; specific errors (e.g., feedback not found on upvote/delete) are not handled distinctly. + +## `src/lib/server/services/` + +- **Notes:** Contains service classes (`practicePlanService.js`, `drillService.js`, etc.) that encapsulate business logic and database interactions for different entities. They extend a `baseEntityService.js`. These were reviewed in detail previously (see `service-notes.md` and `practice-plan-notes.md`). +- **Potential Issues:** Key issues noted previously include scalability problems (lack of pagination/filtering in `getAll` methods), complex update strategies (delete-then-insert), reliance on specific error message strings, and the critical importance of the `BaseEntityService` implementation. Refer to `service-notes.md` and `practice-plan-notes.md` for full details. + +## `src/lib/stores/cartStore.js` + +- **Notes:** Implements a custom Svelte store for managing a 'cart' of drills. Uses a factory function `createCartStore` to encapsulate logic. Initializes state from `localStorage` ('cartDrills') if available, ensuring persistence across browser sessions. Provides methods `addDrill` (adds if not present), `removeDrill` (removes by ID), `toggleDrill` (adds or removes), and `clear` (empties the cart). Updates `localStorage` on modifications. +- **Potential Issues:** + - **Client-Side Only:** Relies entirely on `localStorage`, meaning the cart is specific to the browser and not persistent across devices or linked to a user account on the server. This might be the intended behavior for a temporary selection mechanism. + - **Error Handling:** Doesn't include specific error handling for `localStorage` operations (e.g., quota exceeded, security restrictions), though these are generally rare in modern browsers for small amounts of data. + - **Structure:** Stores the entire drill objects in the cart. Depending on object size and cart capacity, storing only IDs might be slightly more efficient, requiring re-fetching details when needed. The current approach is simpler. + +## `src/lib/stores/dragManager.js` + +- **Notes:** Manages drag-and-drop state and logic primarily for adding items _from the drill search/cart_ into the practice plan sections. It handles `dragstart` from the source (e.g., cart) and `dragover`/`drop` onto sections or items within the plan builder. It interacts with `sectionsStore` to add the new item at the correct position. (Reviewed in `practice-plan-wizard-notes.md`). +- **Potential Issues:** This module replaced the old `dragStore.js` (now removed) and focuses on adding _new_ items rather than reordering existing ones within the plan. + +## `src/lib/stores/dragStore.js` (removed) + +`dragStore.js` previously attempted to manage drag state for reordering items within a practice plan. The file was unused and has since been deleted. + +## `src/lib/stores/drillsStore.js` + +- **Notes:** Manages state related to the drills list page, including filters (`selectedSkillLevels`, `selectedComplexities`, `selectedSkillsFocusedOn`, etc.), search query (`searchQuery`), pagination (`currentPage`, `drillsPerPage`, etc.), and the list of drills itself (`drills`). Initializes `allSkills` from `PREDEFINED_SKILLS`. (Reviewed in `drill-notes.md`). +- **Potential Issues:** Primarily designed for client-side filtering/pagination state management; server-side implementation needed for scalability. + +## `src/lib/stores/feedbackStore.js` + +`feedbackStore.js` now only exports `feedbackModalVisible` for controlling the feedback modal. + + +## `src/lib/stores/formationsStore.js` + +- **Notes:** Manages state related to browsing and filtering formations. Includes `writable` stores for pagination (`currentPage`, `formationsPerPage`, `totalPages`, `totalItems`), loading state (`isLoading`), formation data (`formations`), filters (`selectedTags`, `searchQuery`, `selectedFormationType`), and sorting (`selectedSortOption`, `selectedSortOrder`). Provides `initializeFormations` and `resetFormationFilters` functions. +- **Potential Issues:** + - **Client-Side Focus:** Primarily designed for client-side state management. Scalability requires server-side filtering/sorting/pagination. + - **Usage:** Used in `src/routes/formations/+page.svelte`. + +## `src/lib/stores/historyStore.js` + +- **Notes:** Implements undo/redo functionality for the practice plan editor. Maintains `commandHistory` and `redoStack` arrays. Stores action `type`, `payload`, `description`, and a deep `snapshot` of the `$sections` store state before the action. Provides `addToHistory`, `undo`, `redo` functions. Uses `canUndo`/`canRedo` stores for UI state. Integrates with `svelte-toast`. +- **Potential Issues:** + - **Complexity & State Dependency:** Undo/redo for the complex `$sections` state is difficult. Relies on deep copying via `JSON.stringify`. + - **Snapshot Target:** Only snapshots `$sections`. Other state (e.g., plan metadata) is not included in undo/redo. + - **Performance:** Deep copying large state via `JSON.stringify` on every action could be slow for very large plans. + - **`withHistory` Wrapper:** Previously unused; this helper was removed during cleanup. + +## `src/lib/stores/practicePlanStore.js` + +- **Notes:** Manages state for the `PracticePlanForm.svelte`. Includes stores for form fields, UI state, practice goals. Provides functions for initialization, validation, and submission (`submitPracticePlan` which calls API). Calculates total duration derived from `sectionsStore`. (Reviewed in detail in `practice-plan-notes.md`). +- **Potential Issues:** Very large and mixes many concerns (form state, API logic, UI state, utilities, unrelated filter/drag-drop helpers). Needs significant refactoring for better separation of concerns. Relies on fragile `sessionStorage` auth flow. Validation could be improved. + +## `src/lib/stores/sectionsStore.js` + +- **Notes:** Manages the core `sections` array for the practice plan builder (nested structure of sections and items). Includes state/logic for timeline management and parallel groups. Provides many functions for manipulating the structure (add/remove/update sections, items, groups, timelines). Calculates durations. Integrates with `historyStore`. (Reviewed in detail in `practice-plan-notes.md`). +- **Potential Issues:** Extremely complex (>1000 lines) due to managing intricate nested state (especially parallel groups). High risk of bugs. Relies on careful immutable updates. Critical data formatting/initialization logic is complex. Undo/redo integration is challenging. Mixes some UI side effects (toasts) with state logic. `selectedItems` store might be unused. Uses module-level counters/non-standard IDs. + +## `src/lib/stores/sortStore.js` + +- **Notes:** Simple store with `selectedSortOption` and `selectedSortOrder` writables. +- **Potential Issues:** + - **Scope:** Generically named; might need refinement if sorting logic diverges across different views. Fine for now. + - **Usage:** Used in `FilterPanel.svelte` and `practice-plans/+page.svelte`. + +## `src/lib/stores/wizardStore.js` & `wizardValidation.js` + +- **Notes:** Manages state (`wizardStore.js`) and validation logic (`wizardValidation.js`) for the multi-step practice plan creation wizard. (Reviewed in `practice-plan-wizard-notes.md`). +- **Potential Issues:** Wizard state management involves coordinating state across multiple steps. Validation logic is separated but tightly coupled to the wizard flow. + +## `src/lib/utils/diagramMigration.js` + +- **Notes:** Provides `fabricToExcalidraw` to convert Fabric.js JSON to Excalidraw format. Handles `line`, `circle`, `textbox`. Includes basic error handling. +- **Potential Issues:** + - **Completeness:** Only handles a subset of Fabric.js types. Others will be ignored. + - **One-Off Utility:** Likely obsolete after data migration is complete. Should be documented as such or removed post-migration. + +## `src/lib/utils/loggerUtils.js` + +- **Notes:** Provides client-side logging utilities (`simplifyForLogging`, `logState`, `logDebug`, `logError`) aiming for consistency and sanitization. +- **Potential Issues:** + - **Unused Code:** Appears **completely unused**. Components/stores use `console.log` directly. Either integrate or **remove**. + - **`simplifyForLogging` Logic:** Simplification logic is specific to certain data structures (drills, sections) and restrictive for others. + - **Hardcoded Prefix:** `logState` uses a hardcoded `[PracticePlanForm]` prefix. + +## `src/lib/vitals.js` + +- **Notes:** Exports an empty `webVitals` function (SvelteKit hook) with a `console.log` indicating tracking is disabled. +- **Potential Issues:** + - **Unused/Disabled:** Web vitals reporting is disabled. Implement if needed, otherwise potentially remove (though SvelteKit might look for the file). + + diff --git a/code-review/modal-notes.md b/code-review/modal-notes.md new file mode 100644 index 00000000..94fbaf56 --- /dev/null +++ b/code-review/modal-notes.md @@ -0,0 +1,98 @@ +# Modal Code Review Notes + +This file contains notes related to the review of modal components and the files that interact with them. + +## `src/components/practice-plan/modals/DrillSearchModal.svelte` + +- **Overall:** The component provides functionality to add existing drills, one-off activities, or breaks to a specific section of a practice plan. It includes a search feature for finding drills. +- **Functionality:** + - Handles adding drills, breaks, and one-off items correctly, interacting with the `sectionsStore`. + - Includes basic error handling for the search API call using `svelte-toast`. + - Resets state appropriately when closed. +- **Potential Improvements:** + - **Debounce Search Input:** The search API is called on every keystroke (`on:input`). Debouncing this would prevent excessive API calls while the user is typing. Consider adding a delay (e.g., 300ms) before triggering the search. + - **Loading State:** There's no visual indicator while the search results are being fetched. Adding a spinner or loading message would improve user experience. + - **Accessibility (A11y):** While basic structure seems okay, review for proper focus management (trapping focus within the modal), keyboard navigation (especially for the search results list), and ARIA attributes (`aria-modal`, `aria-labelledby`, etc.) is recommended. + - **Hardcoded Styles:** Width (`w-[32rem]`), top offset (`top-20`), max-height (`max-h-[400px]`) are hardcoded. Consider using theme variables or constants if applicable. + - **Tight Coupling:** The modal directly imports and uses functions from `sectionsStore`. This is functional but creates tight coupling. An alternative could be dispatching events with the selected drill/item data and letting the parent component handle the store interaction, which might be slightly more reusable but also more verbose. + +## `src/components/practice-plan/modals/EmptyCartModal.svelte` + +- **Overall:** A simple modal that informs the user their cart is empty and prompts them to navigate to the drills page or close the modal. +- **Functionality:** + - Uses a `show` prop for visibility. + - Provides buttons to navigate to `/drills` or close the modal. + - Uses `$app/navigation`'s `goto` for navigation. +- **Potential Improvements:** + - **Dispatch Close Event:** Similar to `DrillSearchModal`, consider dispatching a `close` event instead of relying solely on two-way binding (`bind:show`). This makes the component's intent clearer and decouples it slightly from the parent. The parent would listen for `on:close` and update its state. + - **Accessibility (A11y):** Ensure proper focus management (trapping focus within the modal), keyboard navigation, and ARIA attributes (`aria-modal`, `aria-labelledby`, `aria-describedby`). + - **Hardcoded Styles:** Width (`w-96`) and top offset (`top-20`) are hardcoded. + - **Loading State:** There's no visual indicator while the modal is being fetched. Adding a spinner or loading message would improve user experience. + +## `src/components/practice-plan/modals/TimelineSelectorModal.svelte` + +- **Overall:** Allows users to select which parallel timelines are active for a practice plan section, rename them, and change their associated colors. Relies heavily on the `sectionsStore` for state. +- **Functionality:** + - Uses the `selectedTimelines` store (a Set) to track active timelines. + - Allows editing timeline names and colors via `updateTimelineName` and `updateTimelineColor` from the store. + - Uses `getTimelineName` and `getTimelineColor` to display current state. + - Calls `handleTimelineSave` (presumably from the store) when saving changes. + - Includes small inline dialogs for name editing and color picking. +- **Potential Issues & Improvements:** + - **State Management Complexity:** The component directly manipulates the imported `PARALLEL_TIMELINES` object and uses a local cache (`timelineNamesCache`) along with reactive statements (`$:`) and even a `setTimeout` to try and force UI updates, particularly for timeline names. This suggests the state management approach might be overly complex or fighting Svelte's reactivity system. Direct mutation of imported objects is generally discouraged; actions should ideally be dispatched to the store, which then updates its own state, triggering reactivity naturally. + - **Reactivity Workarounds:** The presence of `setTimeout` and manual object recreation (`timelineNamesStore = { ...timelineNamesStore };`) to trigger updates are strong indicators of potential issues in how state changes are propagated or how components are reacting to store updates. This should be refactored for a cleaner approach relying on Svelte's built-in reactivity. + - **Tight Coupling:** Like `DrillSearchModal`, this modal is tightly coupled to `sectionsStore`, importing many specific functions and variables. Refactoring to use dispatched events for actions like 'save', 'updateName', 'updateColor' could improve decoupling, though it might increase boilerplate. + - **Accessibility (A11y):** + - Ensure focus is trapped within the modal. + - The color picker needs to be keyboard navigable and selectable. Buttons made of `div`s or simple elements need `role="button"` and keyboard event handlers (Enter/Space). + - The "Rename" input field should have an associated `