Fixes/request deduplication #44
@@ -1,592 +0,0 @@
|
||||
# Git Workflow and Branching Strategy
|
||||
|
||||
This document outlines the git workflow, branching strategy, commit conventions, and code review guidelines for the glyphdiff.com project.
|
||||
|
||||
## Table of Contents
|
||||
|
||||
1. [Branching Strategy](#branching-strategy)
|
||||
2. [Branch Naming Conventions](#branch-naming-conventions)
|
||||
3. [Commit Message Conventions](#commit-message-conventions)
|
||||
4. [Code Splitting and Merge Request Guidelines](#code-splitting-and-merge-request-guidelines)
|
||||
5. [Branch Protection Rules](#branch-protection-rules)
|
||||
6. [Git Hooks Configuration](#git-hooks-configuration)
|
||||
|
||||
---
|
||||
|
||||
## Branching Strategy
|
||||
|
||||
We use a Gitflow-inspired branching strategy adapted for our development workflow. This strategy provides a clear structure for feature development, bug fixes, and releases.
|
||||
|
||||
### Branch Types
|
||||
|
||||
#### 1. `main` Branch
|
||||
- **Purpose**: Production-ready code only
|
||||
- **Protection**: Highest level of protection
|
||||
- **Rules**:
|
||||
- Only merge `release/*` or `hotfix/*` branches into `main`
|
||||
- No direct commits allowed
|
||||
- Must pass all tests and code reviews
|
||||
- Tags are created from this branch for releases (e.g., `v1.0.0`)
|
||||
|
||||
#### 2. `develop` Branch
|
||||
- **Purpose**: Integration branch for features
|
||||
- **Protection**: High level of protection
|
||||
- **Rules**:
|
||||
- Merge `feature/*` and `fix/*` branches into `develop`
|
||||
- No direct commits allowed
|
||||
- Must pass all tests before merging
|
||||
- Serves as the base for `release/*` branches
|
||||
|
||||
#### 3. `feature/*` Branches
|
||||
- **Purpose**: Develop new features
|
||||
- **Naming**: `feature/feature-name` (e.g., `feature/font-catalog`, `feature/comparison-grid`)
|
||||
- **Base**: Always branch from `develop`
|
||||
- **Merge**: Merge back into `develop` via Merge Request (MR)
|
||||
- **Rules**:
|
||||
- One feature per branch
|
||||
- Keep branches focused and small
|
||||
- Delete after merging
|
||||
|
||||
#### 4. `fix/*` Branches
|
||||
- **Purpose**: Fix bugs discovered during development
|
||||
- **Naming**: `fix/issue-description` (e.g., `fix/font-loading-error`, `fix/responsive-layout`)
|
||||
- **Base**: Branch from `develop`
|
||||
- **Merge**: Merge back into `develop` via MR
|
||||
- **Rules**:
|
||||
- One fix per branch
|
||||
- Include tests that verify the fix
|
||||
- Delete after merging
|
||||
|
||||
#### 5. `hotfix/*` Branches
|
||||
- **Purpose**: Critical fixes for production issues
|
||||
- **Naming**: `hotfix/critical-fix` (e.g., `hotfix/security-patch`, `hotfix-production-crash`)
|
||||
- **Base**: Branch from `main`
|
||||
- **Merge**: Merge into both `main` and `develop`
|
||||
- **Rules**:
|
||||
- Use only for production emergencies
|
||||
- Must be thoroughly tested
|
||||
- Create a release tag after merging to `main`
|
||||
|
||||
#### 6. `release/*` Branches
|
||||
- **Purpose**: Prepare for a new release
|
||||
- **Naming**: `release/vX.Y.Z` (e.g., `release/v1.0.0`, `release/v1.1.0`)
|
||||
- **Base**: Branch from `develop`
|
||||
- **Merge**: Merge into both `main` and `develop`
|
||||
- **Rules**:
|
||||
- Finalize release notes
|
||||
- Update version numbers
|
||||
- Perform final testing
|
||||
- Create release tag after merging to `main`
|
||||
|
||||
### Branch Workflow Diagram
|
||||
|
||||
```
|
||||
main (production)
|
||||
↑
|
||||
│ hotfix/*, release/*
|
||||
│
|
||||
develop (integration)
|
||||
↑
|
||||
│ feature/*, fix/*
|
||||
│
|
||||
feature branches
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Branch Naming Conventions
|
||||
|
||||
### Feature Branches
|
||||
- Format: `feature/feature-name`
|
||||
- Examples:
|
||||
- `feature/font-catalog`
|
||||
- `feature/comparison-grid`
|
||||
- `feature/dark-mode`
|
||||
- `feature/google-fonts-integration`
|
||||
|
||||
### Fix Branches
|
||||
- Format: `fix/issue-description`
|
||||
- Examples:
|
||||
- `fix/font-loading-error`
|
||||
- `fix/responsive-layout`
|
||||
- `fix/state-persistence`
|
||||
- `fix-accessibility-contrast`
|
||||
|
||||
### Hotfix Branches
|
||||
- Format: `hotfix/critical-fix`
|
||||
- Examples:
|
||||
- `hotfix/security-patch`
|
||||
- `hotfix-production-crash`
|
||||
- `hotfix-api-rate-limit`
|
||||
|
||||
### Release Branches
|
||||
- Format: `release/vX.Y.Z`
|
||||
- Examples:
|
||||
- `release/v1.0.0`
|
||||
- `release/v1.1.0`
|
||||
- `release/v2.0.0`
|
||||
|
||||
### Naming Guidelines
|
||||
- Use lowercase letters
|
||||
- Use hyphens to separate words
|
||||
- Be descriptive but concise
|
||||
- Avoid special characters (except hyphens)
|
||||
- Keep names under 50 characters
|
||||
|
||||
---
|
||||
|
||||
## Commit Message Conventions
|
||||
|
||||
We follow the [Conventional Commits](https://www.conventionalcommits.org/) specification. This format enables automated changelog generation and better commit history readability.
|
||||
|
||||
### Format
|
||||
|
||||
```
|
||||
<type>(<scope>): <subject>
|
||||
|
||||
<body>
|
||||
|
||||
<footer>
|
||||
```
|
||||
|
||||
### Commit Types
|
||||
|
||||
| Type | Description | Examples |
|
||||
|------|-------------|----------|
|
||||
| `feat` | New feature | `feat(fonts): add Google Fonts integration` |
|
||||
| `fix` | Bug fix | `fix(comparison): resolve font loading race condition` |
|
||||
| `docs` | Documentation changes | `docs(readme): update installation instructions` |
|
||||
| `style` | Code style changes (formatting, etc.) | `style(components): format with Prettier` |
|
||||
| `refactor` | Code refactoring | `refactor(stores): simplify state management` |
|
||||
| `test` | Adding or updating tests | `test(fonts): add unit tests for font mapper` |
|
||||
| `chore` | Maintenance tasks | `chore(deps): update Tailwind CSS to v4.0` |
|
||||
| `perf` | Performance improvements | `perf(catalog): implement lazy loading for fonts` |
|
||||
|
||||
### Scope
|
||||
|
||||
The scope provides context about which part of the codebase is affected. Common scopes for this project:
|
||||
|
||||
- `fonts` - Font-related functionality
|
||||
- `comparison` - Font comparison features
|
||||
- `catalog` - Font catalog pages
|
||||
- `stores` - State management stores
|
||||
- `components` - UI components
|
||||
- `routes` - SvelteKit routes
|
||||
- `services` - External API services
|
||||
- `utils` - Utility functions
|
||||
- `types` - TypeScript type definitions
|
||||
- `ui` - UI-related changes (theme, layout, etc.)
|
||||
- `config` - Configuration files
|
||||
|
||||
### Subject
|
||||
|
||||
- Use imperative mood ("add" not "added", "fix" not "fixed")
|
||||
- Keep it short (50 characters or less)
|
||||
- Don't end with a period
|
||||
- Be specific and descriptive
|
||||
|
||||
### Body
|
||||
|
||||
- Use imperative mood
|
||||
- Explain **what** and **why**, not **how**
|
||||
- Wrap at 72 characters
|
||||
- Include references to issues (e.g., `Closes #123`)
|
||||
|
||||
### Footer
|
||||
|
||||
- Reference breaking changes with `BREAKING CHANGE:`
|
||||
- Reference issues with `Closes #123` or `Fixes #456`
|
||||
- Include co-authors if needed
|
||||
|
||||
### Examples
|
||||
|
||||
#### Feature Commit
|
||||
```
|
||||
feat(fonts): add Google Fonts API integration
|
||||
|
||||
Implement Google Fonts API service to fetch and display available fonts.
|
||||
This includes the fetchGoogleFonts function and font mapper utilities.
|
||||
|
||||
Closes #12
|
||||
```
|
||||
|
||||
#### Bug Fix Commit
|
||||
```
|
||||
fix(comparison): resolve font loading race condition
|
||||
|
||||
The comparison grid was attempting to render fonts before they were fully
|
||||
loaded. Added loading state checks to prevent this issue.
|
||||
|
||||
Fixes #45
|
||||
```
|
||||
|
||||
#### Refactor Commit
|
||||
```
|
||||
refactor(stores): simplify state management with Svelte 5 runes
|
||||
|
||||
Migrated from Svelte stores to Svelte 5's $state runes for better
|
||||
performance and simpler code. This change affects all stores in the
|
||||
project.
|
||||
|
||||
BREAKING CHANGE: Store API has changed from subscribe() to direct
|
||||
property access. Update all store consumers accordingly.
|
||||
```
|
||||
|
||||
#### Documentation Commit
|
||||
```
|
||||
docs(git-workflow): add commit message conventions
|
||||
|
||||
Document the conventional commits format with examples and guidelines
|
||||
for the team.
|
||||
```
|
||||
|
||||
#### Chore Commit
|
||||
```
|
||||
chore(deps): update Tailwind CSS to v4.0.0
|
||||
|
||||
Update Tailwind CSS to the latest version and adjust configuration
|
||||
files accordingly.
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Code Splitting and Merge Request Guidelines
|
||||
|
||||
### Merge Request Size Guidelines
|
||||
|
||||
- **Maximum MR size**: < 500 lines changed (additions + deletions)
|
||||
- **Ideal MR size**: 100-300 lines changed
|
||||
- **Files per MR**: < 10 files
|
||||
|
||||
### When to Split a Feature into Multiple MRs
|
||||
|
||||
Split a feature into multiple MRs when:
|
||||
|
||||
1. **The feature is large** (> 500 lines or > 10 files)
|
||||
2. **Multiple concerns are involved** (e.g., UI + API + state management)
|
||||
3. **Independent parts can be tested separately**
|
||||
4. **The feature has logical phases** (e.g., setup → implementation → polish)
|
||||
|
||||
### Example: Splitting a Feature
|
||||
|
||||
**Feature**: Font Catalog with Filtering
|
||||
|
||||
**MR 1**: `feature/font-catalog-setup`
|
||||
- Create basic catalog page structure
|
||||
- Set up routing
|
||||
- Add placeholder components
|
||||
- ~150 lines
|
||||
|
||||
**MR 2**: `feature/font-catalog-data`
|
||||
- Implement Google Fonts API integration
|
||||
- Create font data fetching logic
|
||||
- Add font mapper utilities
|
||||
- ~200 lines
|
||||
|
||||
**MR 3**: `feature/font-catalog-ui`
|
||||
- Build FontCard component
|
||||
- Implement grid layout
|
||||
- Add loading states
|
||||
- ~250 lines
|
||||
|
||||
**MR 4**: `feature/font-catalog-filtering`
|
||||
- Implement filter store
|
||||
- Build FilterBar component
|
||||
- Connect filters to catalog
|
||||
- ~180 lines
|
||||
|
||||
### Merge Request Description Template
|
||||
|
||||
Every MR must include a comprehensive description:
|
||||
|
||||
```markdown
|
||||
## Description
|
||||
Brief description of what this MR changes and why.
|
||||
|
||||
## Changes Made
|
||||
- [ ] Change 1
|
||||
- [ ] Change 2
|
||||
- [ ] Change 3
|
||||
|
||||
## Type of Change
|
||||
- [ ] Bug fix
|
||||
- [ ] New feature
|
||||
- [ ] Breaking change
|
||||
- [ ] Documentation update
|
||||
- [ ] Refactoring
|
||||
- [ ] Performance improvement
|
||||
|
||||
## Testing
|
||||
- [ ] Unit tests pass
|
||||
- [ ] Manual testing completed
|
||||
- [ ] Tested on Chrome
|
||||
- [ ] Tested on Firefox
|
||||
- [ ] Tested on Safari
|
||||
- [ ] Tested on mobile (responsive)
|
||||
|
||||
## Screenshots (if applicable)
|
||||
Add screenshots or GIFs showing the changes.
|
||||
|
||||
## Checklist
|
||||
- [ ] Code follows project style guidelines
|
||||
- [ ] Self-review completed
|
||||
- [ ] Comments added for complex logic
|
||||
- [ ] Documentation updated
|
||||
- [ ] No new warnings generated
|
||||
- [ ] Tests added/updated
|
||||
- [ ] All tests passing
|
||||
|
||||
## Related Issues
|
||||
Closes #123
|
||||
Related to #456
|
||||
```
|
||||
|
||||
### Code Review Checklist
|
||||
|
||||
Reviewers should check:
|
||||
|
||||
#### Functionality
|
||||
- [ ] Does the code work as intended?
|
||||
- [ ] Are edge cases handled?
|
||||
- [ ] Is error handling appropriate?
|
||||
|
||||
#### Code Quality
|
||||
- [ ] Is the code readable and maintainable?
|
||||
- [ ] Are variable/function names descriptive?
|
||||
- [ ] Is there unnecessary complexity?
|
||||
- [ ] Are there code duplications?
|
||||
|
||||
#### Best Practices
|
||||
- [ ] Does it follow project conventions?
|
||||
- [ ] Are TypeScript types properly defined?
|
||||
- [ ] Are Svelte best practices followed?
|
||||
- [ ] Is Tailwind CSS used appropriately?
|
||||
|
||||
#### Testing
|
||||
- [ ] Are tests included?
|
||||
- [ ] Do tests cover edge cases?
|
||||
- [ ] Are tests meaningful and not redundant?
|
||||
|
||||
#### Documentation
|
||||
- [ ] Is the code self-documenting?
|
||||
- [ ] Are complex functions commented?
|
||||
- [ ] Is the MR description clear?
|
||||
|
||||
#### Performance
|
||||
- [ ] Are there performance concerns?
|
||||
- [ ] Is lazy loading used where appropriate?
|
||||
- [ ] Are unnecessary re-renders avoided?
|
||||
|
||||
### Merge Request Approval Process
|
||||
|
||||
1. **Author**: Creates MR with complete description
|
||||
2. **Reviewer**: Reviews code using the checklist above
|
||||
3. **Discussion**: Address any concerns or suggestions
|
||||
4. **Approval**: At least one approval required
|
||||
4. **Merge**: Squash and merge into target branch
|
||||
5. **Cleanup**: Delete source branch after merge
|
||||
|
||||
---
|
||||
|
||||
## Branch Protection Rules
|
||||
|
||||
### `main` Branch Protection
|
||||
|
||||
- **Require pull request reviews**: Yes
|
||||
- Required approvers: 1
|
||||
- Dismiss stale reviews: Yes
|
||||
- **Require status checks**: Yes
|
||||
- Required checks: All tests, linting
|
||||
- Require branches to be up to date: Yes
|
||||
- **Restrict who can push**: Only maintainers
|
||||
- **Require linear history**: Yes (squash and merge)
|
||||
- **Block force pushes**: Yes
|
||||
|
||||
### `develop` Branch Protection
|
||||
|
||||
- **Require pull request reviews**: Yes
|
||||
- Required approvers: 1
|
||||
- Dismiss stale reviews: Yes
|
||||
- **Require status checks**: Yes
|
||||
- Required checks: All tests, linting
|
||||
- Require branches to be up to date: Yes
|
||||
- **Restrict who can push**: Only developers and maintainers
|
||||
- **Require linear history**: Yes (squash and merge)
|
||||
- **Block force pushes**: Yes
|
||||
|
||||
### Implementation Notes
|
||||
|
||||
These rules should be configured in your Git hosting platform (GitHub, GitLab, or Bitbucket). The exact configuration steps vary by platform:
|
||||
|
||||
- **GitHub**: Settings → Branches → Add rule
|
||||
- **GitLab**: Settings → Repository → Protected branches
|
||||
- **Bitbucket**: Repository settings → Branch restrictions
|
||||
|
||||
---
|
||||
|
||||
## Git Hooks Configuration
|
||||
|
||||
Git hooks are automated scripts that run at specific points in the git workflow. They help maintain code quality and consistency.
|
||||
|
||||
### Recommended Hooks
|
||||
|
||||
#### 1. Pre-commit Hook
|
||||
**Purpose**: Run linter and formatter before committing
|
||||
|
||||
**Tools**: ESLint, Prettier
|
||||
|
||||
**Implementation**:
|
||||
```bash
|
||||
#!/bin/sh
|
||||
# .git/hooks/pre-commit
|
||||
|
||||
# Run Prettier
|
||||
npm run format:check
|
||||
|
||||
# Run ESLint
|
||||
npm run lint
|
||||
|
||||
# Exit with error if any check fails
|
||||
if [ $? -ne 0 ]; then
|
||||
echo "❌ Pre-commit checks failed. Please fix the issues before committing."
|
||||
exit 1
|
||||
fi
|
||||
|
||||
echo "✅ Pre-commit checks passed."
|
||||
```
|
||||
|
||||
**Setup**:
|
||||
```bash
|
||||
# Install husky (recommended)
|
||||
npm install --save-dev husky
|
||||
|
||||
# Initialize husky
|
||||
npx husky install
|
||||
|
||||
# Add pre-commit hook
|
||||
npx husky add .husky/pre-commit "npm run lint && npm run format:check"
|
||||
```
|
||||
|
||||
#### 2. Commit-msg Hook
|
||||
**Purpose**: Validate commit message format
|
||||
|
||||
**Tools**: commitlint
|
||||
|
||||
**Implementation**:
|
||||
```bash
|
||||
#!/bin/sh
|
||||
# .git/hooks/commit-msg
|
||||
|
||||
# Validate commit message with commitlint
|
||||
npx --no -- commitlint --edit $1
|
||||
```
|
||||
|
||||
**Setup**:
|
||||
```bash
|
||||
# Install commitlint
|
||||
npm install --save-dev @commitlint/cli @commitlint/config-conventional
|
||||
|
||||
# Create commitlint config
|
||||
echo "module.exports = { extends: ['@commitlint/config-conventional'] };" > commitlint.config.js
|
||||
|
||||
# Add commit-msg hook
|
||||
npx husky add .husky/commit-msg "npx --no -- commitlint --edit \$1"
|
||||
```
|
||||
|
||||
#### 3. Pre-push Hook
|
||||
**Purpose**: Run tests before pushing
|
||||
|
||||
**Tools**: Vitest, SvelteKit test runner
|
||||
|
||||
**Implementation**:
|
||||
```bash
|
||||
#!/bin/sh
|
||||
# .git/hooks/pre-push
|
||||
|
||||
# Run tests
|
||||
npm run test
|
||||
|
||||
# Exit with error if tests fail
|
||||
if [ $? -ne 0 ]; then
|
||||
echo "❌ Tests failed. Please fix the failing tests before pushing."
|
||||
exit 1
|
||||
fi
|
||||
|
||||
echo "✅ All tests passed."
|
||||
```
|
||||
|
||||
**Setup**:
|
||||
```bash
|
||||
# Add pre-push hook
|
||||
npx husky add .husky/pre-push "npm run test"
|
||||
```
|
||||
|
||||
### Alternative: Using Husky
|
||||
|
||||
[Husky](https://typicode.github.io/husky/) is a popular tool for managing git hooks. It's easier to maintain and works across different operating systems.
|
||||
|
||||
**Installation**:
|
||||
```bash
|
||||
npm install --save-dev husky
|
||||
npx husky install
|
||||
npm pkg set scripts.prepare="husky install"
|
||||
```
|
||||
|
||||
**Adding hooks**:
|
||||
```bash
|
||||
# Pre-commit hook
|
||||
npx husky add .husky/pre-commit "npm run lint && npm run format:check"
|
||||
|
||||
# Commit-msg hook
|
||||
npx husky add .husky/commit-msg "npx --no -- commitlint --edit \$1"
|
||||
|
||||
# Pre-push hook
|
||||
npx husky add .husky/pre-push "npm run test"
|
||||
```
|
||||
|
||||
### Hook Scripts for This Project
|
||||
|
||||
Once the project is set up with SvelteKit, add these scripts to `package.json`:
|
||||
|
||||
```json
|
||||
{
|
||||
"scripts": {
|
||||
"format": "prettier --write .",
|
||||
"format:check": "prettier --check .",
|
||||
"lint": "eslint .",
|
||||
"lint:fix": "eslint . --fix",
|
||||
"test": "vitest run",
|
||||
"test:watch": "vitest",
|
||||
"prepare": "husky install"
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### Benefits of Git Hooks
|
||||
|
||||
1. **Consistency**: Enforce code style and formatting
|
||||
2. **Quality**: Catch bugs before they're committed
|
||||
3. **Efficiency**: Fail fast, fix early
|
||||
4. **Automation**: Reduce manual checks
|
||||
5. **Team alignment**: Ensure everyone follows the same standards
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
This git workflow provides a structured approach to development for the glyphdiff.com project:
|
||||
|
||||
- **Clear branching strategy** with defined purposes for each branch type
|
||||
- **Conventional commits** for readable and automated changelogs
|
||||
- **Code splitting guidelines** to keep MRs focused and reviewable
|
||||
- **Comprehensive review process** to maintain code quality
|
||||
- **Git hooks** to automate quality checks
|
||||
|
||||
Following this workflow will help the team:
|
||||
- Develop features in parallel without conflicts
|
||||
- Maintain a clean git history
|
||||
- Catch issues early in the development process
|
||||
- Ensure code quality and consistency
|
||||
- Streamline the release process
|
||||
|
||||
For questions or suggestions about this workflow, please discuss with the team or create an issue in the project repository.
|
||||
@@ -21,6 +21,7 @@ vi.mock('$shared/api/api', () => ({
|
||||
import { api } from '$shared/api/api';
|
||||
import { queryClient } from '$shared/api/queryClient';
|
||||
import { fontKeys } from '$shared/api/queryKeys';
|
||||
import { FontResponseError } from '../../lib/errors/errors';
|
||||
import {
|
||||
fetchFontsByIds,
|
||||
fetchProxyFontById,
|
||||
@@ -86,16 +87,20 @@ describe('proxyFonts', () => {
|
||||
expect(calledUrl).toContain('offset=0');
|
||||
});
|
||||
|
||||
test('should throw on invalid response (missing fonts array)', async () => {
|
||||
test('should throw FontResponseError on invalid response (missing fonts array)', async () => {
|
||||
mockApiGet({ total: 0 });
|
||||
|
||||
await expect(fetchProxyFonts()).rejects.toThrow('Proxy API returned invalid response');
|
||||
await expect(fetchProxyFonts()).rejects.toSatisfy(
|
||||
e => e instanceof FontResponseError && e.field === 'response.fonts',
|
||||
);
|
||||
});
|
||||
|
||||
test('should throw on null response data', async () => {
|
||||
test('should throw FontResponseError on null response data', async () => {
|
||||
vi.mocked(api.get).mockResolvedValueOnce({ data: null, status: 200 });
|
||||
|
||||
await expect(fetchProxyFonts()).rejects.toThrow('Proxy API returned invalid response');
|
||||
await expect(fetchProxyFonts()).rejects.toSatisfy(
|
||||
e => e instanceof FontResponseError && e.field === 'response',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -15,6 +15,7 @@ import { queryClient } from '$shared/api/queryClient';
|
||||
import { fontKeys } from '$shared/api/queryKeys';
|
||||
import { buildQueryString } from '$shared/lib/utils';
|
||||
import type { QueryParams } from '$shared/lib/utils';
|
||||
import { FontResponseError } from '../../lib/errors/errors';
|
||||
import type { UnifiedFont } from '../../model/types';
|
||||
|
||||
/**
|
||||
@@ -96,11 +97,16 @@ export interface ProxyFontsParams extends QueryParams {
|
||||
/**
|
||||
* Proxy API response
|
||||
*
|
||||
* Includes pagination metadata alongside font data
|
||||
* Includes pagination metadata alongside font data.
|
||||
*
|
||||
* Contract: `fonts` is always an array — never `null` or omitted, even when
|
||||
* `total === 0`. Returning `null` on the wire is a backend regression and
|
||||
* surfaces as FontResponseError (non-retryable) on the client.
|
||||
*/
|
||||
export interface ProxyFontsResponse {
|
||||
/**
|
||||
* List of font objects returned by the proxy
|
||||
* List of font objects returned by the proxy.
|
||||
* Always an array; empty when no matches.
|
||||
*/
|
||||
fonts: UnifiedFont[];
|
||||
|
||||
@@ -156,8 +162,11 @@ export async function fetchProxyFonts(
|
||||
|
||||
const response = await api.get<ProxyFontsResponse>(url);
|
||||
|
||||
if (!response.data || !Array.isArray(response.data.fonts)) {
|
||||
throw new Error('Proxy API returned invalid response');
|
||||
if (!response.data) {
|
||||
throw new FontResponseError('response', response.data);
|
||||
}
|
||||
if (!Array.isArray(response.data.fonts)) {
|
||||
throw new FontResponseError('response.fonts', response.data.fonts);
|
||||
}
|
||||
|
||||
return response.data;
|
||||
|
||||
@@ -1,3 +1,5 @@
|
||||
import { NonRetryableError } from '$shared/api/queryClient';
|
||||
|
||||
/**
|
||||
* Thrown when the network request to the proxy API fails.
|
||||
* Wraps the underlying fetch error (timeout, DNS failure, connection refused, etc.).
|
||||
@@ -12,11 +14,13 @@ export class FontNetworkError extends Error {
|
||||
|
||||
/**
|
||||
* Thrown when the proxy API returns a response with an unexpected shape.
|
||||
* Extends NonRetryableError because schema mismatches are not transient —
|
||||
* retrying will produce the same failure and only delay surfacing the bug.
|
||||
*
|
||||
* @property field - The name of the field that failed validation (e.g. `'response'`, `'response.fonts'`).
|
||||
* @property received - The actual value received at that field, for debugging.
|
||||
*/
|
||||
export class FontResponseError extends Error {
|
||||
export class FontResponseError extends NonRetryableError {
|
||||
readonly name = 'FontResponseError';
|
||||
|
||||
constructor(
|
||||
|
||||
@@ -84,9 +84,10 @@ describe('FontCatalogStore', () => {
|
||||
store.destroy();
|
||||
});
|
||||
|
||||
it('starts with isEmpty false — initial fetch is in progress', () => {
|
||||
// The observer starts fetching immediately on construction.
|
||||
// isEmpty must be false so the UI shows a loader, not "no results".
|
||||
it('starts with isEmpty false — observer is gated until setParams enables it', () => {
|
||||
// The observer is disabled on construction (no auto-fetch) — see
|
||||
// `#enabled` in the store. isEmpty must still be false so the UI
|
||||
// doesn't flash "no results" before bindings configures the query.
|
||||
const store = makeStore();
|
||||
expect(store.isEmpty).toBe(false);
|
||||
store.destroy();
|
||||
|
||||
@@ -31,6 +31,13 @@ type FontStoreResult = InfiniteQueryObserverResult<InfiniteData<ProxyFontsRespon
|
||||
|
||||
export class FontCatalogStore {
|
||||
#params = $state<FontStoreParams>({ limit: 50 });
|
||||
/**
|
||||
* Gates the initial fetch. The observer starts disabled so the constructor
|
||||
* cannot race ahead of the bindings module — which is the single source of
|
||||
* truth for query params. The first setParams flips this on, producing a
|
||||
* single fetch with the correctly merged queryKey.
|
||||
*/
|
||||
#enabled = $state(false);
|
||||
#result = $state<FontStoreResult>({} as FontStoreResult);
|
||||
#observer: InfiniteQueryObserver<
|
||||
ProxyFontsResponse,
|
||||
@@ -45,6 +52,8 @@ export class FontCatalogStore {
|
||||
constructor(params: FontStoreParams = {}) {
|
||||
this.#params = { limit: 50, ...params };
|
||||
this.#observer = new InfiniteQueryObserver(this.#qc, this.buildOptions());
|
||||
// Seed result synchronously; subscribe may not fire on disabled observers.
|
||||
this.#result = this.#observer.getCurrentResult();
|
||||
this.#unsubscribe = this.#observer.subscribe(r => {
|
||||
this.#result = r;
|
||||
});
|
||||
@@ -88,10 +97,13 @@ export class FontCatalogStore {
|
||||
return this.#result.error ?? null;
|
||||
}
|
||||
/**
|
||||
* True if no fonts were found for the current filter criteria
|
||||
* True if no fonts were found for the current filter criteria.
|
||||
* Always false until the observer has been enabled (via setParams) — otherwise
|
||||
* the UI would briefly render "no results" on mount before bindings configures
|
||||
* the query.
|
||||
*/
|
||||
get isEmpty(): boolean {
|
||||
return !this.isLoading && !this.isFetching && this.fonts.length === 0;
|
||||
return this.#enabled && !this.isLoading && !this.isFetching && this.fonts.length === 0;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -129,10 +141,12 @@ export class FontCatalogStore {
|
||||
}
|
||||
|
||||
/**
|
||||
* Merge new parameters into existing state and trigger a refetch
|
||||
* Merge new parameters into existing state and trigger a refetch.
|
||||
* The first call also enables the observer (see `#enabled`).
|
||||
*/
|
||||
setParams(updates: Partial<FontStoreParams>) {
|
||||
this.#params = { ...this.#params, ...updates };
|
||||
this.#enabled = true;
|
||||
this.#observer.setOptions(this.buildOptions());
|
||||
}
|
||||
/**
|
||||
@@ -431,6 +445,7 @@ export class FontCatalogStore {
|
||||
const next = lastPage.offset + lastPage.limit;
|
||||
return next < lastPage.total ? { offset: next } : undefined;
|
||||
},
|
||||
enabled: this.#enabled,
|
||||
staleTime: hasFilters ? 0 : DEFAULT_QUERY_STALE_TIME_MS,
|
||||
gcTime: DEFAULT_QUERY_GC_TIME_MS,
|
||||
};
|
||||
@@ -441,6 +456,11 @@ export class FontCatalogStore {
|
||||
try {
|
||||
response = await fetchProxyFonts(params);
|
||||
} catch (cause) {
|
||||
// Preserve non-retryable validation errors so the query client doesn't
|
||||
// burn the retry budget on a deterministic schema mismatch.
|
||||
if (cause instanceof FontResponseError) {
|
||||
throw cause;
|
||||
}
|
||||
throw new FontNetworkError(cause);
|
||||
}
|
||||
|
||||
|
||||
@@ -40,6 +40,10 @@ interface Props extends
|
||||
* Skeleton snippet
|
||||
*/
|
||||
skeleton?: Snippet;
|
||||
/**
|
||||
* Empty-state snippet rendered when the query settled with zero fonts
|
||||
*/
|
||||
empty?: Snippet;
|
||||
}
|
||||
|
||||
let {
|
||||
@@ -47,6 +51,7 @@ let {
|
||||
onVisibleItemsChange,
|
||||
weight,
|
||||
skeleton,
|
||||
empty,
|
||||
...rest
|
||||
}: Props = $props();
|
||||
|
||||
@@ -59,6 +64,8 @@ let isCatchingUp = $state(false);
|
||||
|
||||
const showInitialSkeleton = $derived(!!skeleton && isLoading && fontCatalogStore.fonts.length === 0);
|
||||
const showCatchupSkeleton = $derived(!!skeleton && isCatchingUp);
|
||||
// Settled query with no matches — empty state replaces the (otherwise blank) list.
|
||||
const showEmpty = $derived(!!empty && !isLoading && !isCatchingUp && fontCatalogStore.fonts.length === 0);
|
||||
|
||||
function handleInternalVisibleChange(items: UnifiedFont[]) {
|
||||
visibleFonts = items;
|
||||
@@ -163,6 +170,10 @@ function handleNearBottom(_lastVisibleIndex: number) {
|
||||
<div class="overflow-hidden h-full" transition:fade={{ duration: 300 }}>
|
||||
{@render skeleton()}
|
||||
</div>
|
||||
{:else if showEmpty && empty}
|
||||
<div class="h-full" transition:fade={{ duration: 200 }}>
|
||||
{@render empty()}
|
||||
</div>
|
||||
{:else}
|
||||
<!-- VirtualList persists during pagination - no destruction/recreation -->
|
||||
<VirtualList
|
||||
|
||||
@@ -9,6 +9,7 @@
|
||||
|
||||
import { api } from '$shared/api/api';
|
||||
import { API_ENDPOINTS } from '$shared/api/endpoints';
|
||||
import { NonRetryableError } from '$shared/api/queryClient';
|
||||
|
||||
const PROXY_API_URL = API_ENDPOINTS.filters;
|
||||
|
||||
@@ -37,7 +38,8 @@ export interface FilterMetadata {
|
||||
type: 'enum' | 'string' | 'array';
|
||||
|
||||
/**
|
||||
* Available filter options
|
||||
* Available filter options.
|
||||
* Always an array; empty when the group has no options.
|
||||
*/
|
||||
options: FilterOption[];
|
||||
}
|
||||
@@ -68,11 +70,16 @@ export interface FilterOption {
|
||||
}
|
||||
|
||||
/**
|
||||
* Proxy filters API response
|
||||
* Proxy filters API response.
|
||||
*
|
||||
* Contract: `filters` (and each nested `options`) is always an array — never
|
||||
* `null` or omitted. Wire-level `null` here is a backend regression and
|
||||
* surfaces as a non-retryable error on the client.
|
||||
*/
|
||||
export interface ProxyFiltersResponse {
|
||||
/**
|
||||
* Array of filter metadata
|
||||
* Array of filter metadata.
|
||||
* Always an array; empty when no filter groups are configured.
|
||||
*/
|
||||
filters: FilterMetadata[];
|
||||
}
|
||||
@@ -99,7 +106,7 @@ export async function fetchProxyFilters(): Promise<FilterMetadata[]> {
|
||||
const response = await api.get<FilterMetadata[]>(PROXY_API_URL);
|
||||
|
||||
if (!response.data || !Array.isArray(response.data)) {
|
||||
throw new Error('Proxy API returned invalid response');
|
||||
throw new NonRetryableError('Proxy API returned invalid filters response');
|
||||
}
|
||||
|
||||
return response.data;
|
||||
|
||||
@@ -42,20 +42,19 @@ $effect.root(() => {
|
||||
});
|
||||
|
||||
/**
|
||||
* Mirror filter selections + debounced search query into fontCatalogStore params.
|
||||
* Mirror filter selections + debounced search query + sort into fontCatalogStore params.
|
||||
*
|
||||
* Filters and sort are merged into one setParams call to avoid a startup race:
|
||||
* two separate effects each issued setOptions with a different queryKey on the
|
||||
* first flush, producing an orphaned `?limit=50&offset=0` fetch immediately
|
||||
* followed by the real `?limit=50&sort=popularity&offset=0` fetch.
|
||||
*
|
||||
* untrack the write so fontCatalogStore's internal $state reads don't feed back
|
||||
* into this effect's dependency graph.
|
||||
*/
|
||||
$effect(() => {
|
||||
const params = mapAppliedFiltersToParams(appliedFilterStore);
|
||||
untrack(() => fontCatalogStore.setParams(params));
|
||||
});
|
||||
|
||||
/**
|
||||
* Mirror sort selection into fontCatalogStore.
|
||||
*/
|
||||
$effect(() => {
|
||||
const apiSort = sortStore.apiValue;
|
||||
untrack(() => fontCatalogStore.setSort(apiSort));
|
||||
const sort = sortStore.apiValue;
|
||||
untrack(() => fontCatalogStore.setParams({ ...params, sort }));
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,5 +1,15 @@
|
||||
import { QueryClient } from '@tanstack/query-core';
|
||||
|
||||
/**
|
||||
* Marker base class for errors that retrying will never fix — schema-validation
|
||||
* failures, unauthorized responses, contract violations, etc.
|
||||
*
|
||||
* The queryClient retry handler short-circuits when it sees this; without it,
|
||||
* a non-transient backend bug pins the UI through the full retry budget
|
||||
* (default 3× exponential backoff ≈ 7s).
|
||||
*/
|
||||
export class NonRetryableError extends Error {}
|
||||
|
||||
/**
|
||||
* Data remains fresh for this long after fetch. Stores that override
|
||||
* staleness (e.g. filtered queries) can use 0 to bypass.
|
||||
@@ -51,7 +61,12 @@ export const queryClient = new QueryClient({
|
||||
* Refetch on mount if data is stale
|
||||
*/
|
||||
refetchOnMount: true,
|
||||
retry: QUERY_RETRY_COUNT,
|
||||
retry: (failureCount, error) => {
|
||||
if (error instanceof NonRetryableError) {
|
||||
return false;
|
||||
}
|
||||
return failureCount < QUERY_RETRY_COUNT;
|
||||
},
|
||||
/**
|
||||
* Exponential backoff: 1s, 2s, 4s, 8s... capped at 30s
|
||||
*/
|
||||
|
||||
@@ -104,6 +104,12 @@ function isFontReady(font: UnifiedFont): boolean {
|
||||
gap={2}
|
||||
class="bg-transparent min-h-0 h-full scroll-stable py-2 pl-6 pr-4"
|
||||
>
|
||||
{#snippet empty()}
|
||||
<div class="px-6 py-12 flex items-center justify-center">
|
||||
<Label variant="muted" size="sm">No typefaces found</Label>
|
||||
</div>
|
||||
{/snippet}
|
||||
|
||||
{#snippet skeleton()}
|
||||
<div class="py-2.5 md:py-3 px-7">
|
||||
{#each { length: 50 } as _, index (index)}
|
||||
|
||||
Reference in New Issue
Block a user