Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 67 additions & 28 deletions docs/adr/0001-frontend-layering.md
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
# ADR 0001 — Frontend layering and dependency direction

Status: Proposed
Status: Accepted
Date: 2026-08-26
Supersedes: none
Applies to: `src/components/**`, `src/features/**`, `src/store/**`, `src/services/**`

> Status is **Proposed** until this PR lands; it flips to **Accepted** on merge.
Applies to: `src/components/**`, `src/features/**`, `src/hooks/**`, `src/store/**`, `src/services/**`

## Context

Expand Down Expand Up @@ -74,6 +72,38 @@ Store, or any service — that is exactly the responsibility split the migration
⁵ Services may read Store state through `useAppStore.getState()` (non-reactive) but must not
subscribe reactively. This is the existing pattern; nothing changes here.

### File placement (enforced by `check-boundaries.cjs`)

The dependency table only means something if every module has an unambiguous tier. These
placement rules close the gray zones that existed before they were written down:

- **Hooks** live in `src/features/*/hooks/**`. A `use*.ts(x)` module directly under a feature's
root directory fails `check-boundaries.cjs`. A hook that is genuinely shared across features
lives in `src/hooks/**` (footnote ² above) — importing it from another feature's `hooks/**`
is the sanctioned cross-feature path. (History: `useAuthSessionGeneration` and
`useBackendLifecycle` sat at `src/features/lifecycle/` root; the first hid two cross-feature
hook imports from every check, the second just set a bad example.)
- **Feature-local view components** may live in `src/features/*/components/**`. They are View
tier: the business-service import ban for `src/components/**` applies to them identically,
in both ESLint and `check-boundaries.cjs`.
- **Domain persistence modules** (IndexedDB/localStorage wrappers for one feature's data —
`repositoryChatStorage`, `discoveryAnalysisStorage`, `weeklyIssuesStorage`,
`indexedDbStorage`) live in `src/services/*Storage.ts`. By the rule of thumb below they are
infrastructure (local persistence, no remote calls, no Store mutation), so any tier may
import them. Do not invent new top-level directories inside a feature beyond `hooks/`,
`components/`, `application/`, `__tests__/` without an ADR amendment. (History:
`repository-chat/repositories/` invented an undocumented "repositories" tier whose name also
collided with the `repositories` feature.)

### Page view-model selectors

Selectors that bundle a whole page's state and actions (e.g. `selectReleaseTimelineState`,
`selectDiscoveryViewState` in `src/store/selectors.ts`) are page-scoped contracts: their only
reactive consumer is that page's orchestration hook in `src/features/*/hooks/**`. A component
or hook that needs a subset must not subscribe through them — every field change re-renders
it. Add a narrow single-value selector, or a small hook next to the page's action hooks (see
`useWatchedSourcesSync`), instead.

### Why one persisted Store is retained

The Store is split into *slices* for readability, but there is exactly **one** `create<AppStoreState>()(persist(...))`
Expand Down Expand Up @@ -115,9 +145,12 @@ guarantee for the v2 backend/electron split.

### Infrastructure vs business service — the import carve-out

`no-restricted-imports` (PR 9) bans `src/components/**` from importing these **business services**:
`githubApi`, `aiService`, `aiAnalysisHelper`, `aiAnalysisOptimizer`, `vectorSearchService`,
`autoSync`, `webdavService`, `backendAdapter`, `rpcDownloadService`, `githubApiFactory`.
`no-restricted-imports` (PR 9) bans View components — `src/components/**` and, since the
placement rules above, `src/features/*/components/**` — from importing these **business
services**: `githubApi`, `aiService`, `aiAnalysisHelper`, `aiAnalysisOptimizer`,
`vectorSearchService`, `autoSync`, `webdavService`, `backendAdapter`, `rpcDownloadService`,
`githubApiFactory`, `updateService`, `translateService`. The same list is mirrored in
`scripts/check-boundaries.cjs`; keep the two in sync.

These remain importable from components because they are **tools, not orchestration**:
`logger`, `electronProxy` (`isElectron`), `indexedDbStorage`, `mcpElectronBridge`,
Expand All @@ -127,43 +160,49 @@ These remain importable from components because they are **tools, not orchestrat
> a business service and belongs behind a hook. If it is a sync utility (`logger`, `isElectron`,
> `indexedDBStorage`), it is infrastructure and may be imported anywhere.

This PR's ban list is the ten services above. Two further services — `updateService` and
`translateService` — are business services by the same rule (they make remote calls) but are
*not* banned in this PR; `BilingualMarkdownRenderer`, `UpdateChecker`, and
`UpdateNotificationBanner` still import them directly. They are phased out alongside the
component tail below. The ban list is deliberately the set the migration already covered;
expanding it is a follow-up PR, not this one.
When this ADR landed, the ban list was the first ten services above; `updateService` and
`translateService` (business services by the same rule — they make remote calls) were left
unbanned because `BilingualMarkdownRenderer`, `UpdateChecker`, and `UpdateNotificationBanner`
still imported them directly. That phase is complete: PR #326 migrated the last three
components and folded both services into the ban list, and the allowlist no longer exists.

### Phased enforcement (historical — completed in PR #326)

### Phased enforcement
The rollout was phased so that enforcement never masked a pile of pre-existing violations. This
section records those mechanics; the phase is over and the allowlist no longer exists.

PR 4–8 migrated the high-traffic components (`RepositoryCard`, `RepositoryList`, the settings
panels, the timeline views). A snapshot at the `7337df0` baseline showed 33 components importing
services; after the migration that number is down, but a tail of components (e.g.
services; a tail of components (e.g.
`SearchBar`, `ReadmeModal`, `LoginScreen`, `GistCard`, `ReleaseCard`, `SubscriptionRepoCard`,
`CategorySidebar`, `RepositoryEditModal`, `DebugModeIndicator`, `SettingsPanel`,
`ReleaseSourceSettingsModal`, `GistEditorModal`, `GistDetailModal`) still import business
`ReleaseSourceSettingsModal`, `GistEditorModal`, `GistDetailModal`) still imported business
services directly.

PR 9 enforces the rule only on **already-migrated** component directories and the
`src/components/ui/**` primitives (which should never touch a business service), and leaves the
un-migrated tail on an explicit allowlist for a later PR. One-shot banning all remaining imports
would light up a dozen files at once and force a rushed migration in a boundary-PR — exactly the
"don't mask 33 violations in one go" failure mode. The allowlist is the phasing mechanism; each
later PR that migrates a tail component also removes it from the allowlist.
PR 9 therefore enforced the rule only on **already-migrated** component directories and the
`src/components/ui/**` primitives (which should never touch a business service), leaving the
un-migrated tail on an explicit allowlist: one-shot banning all remaining imports would have
forced a rushed migration in a boundary-PR — exactly the
"don't mask 33 violations in one go" failure mode. Each later PR migrated a tail component and
removed it from the allowlist, until PR #326 migrated the last three, folded `updateService` and
`translateService` into the ban list, and emptied the allowlist.

## Consequences

- New components in migrated directories that try to import a business service fail lint **and**
the `check-boundaries.cjs` CI step.
the `check-boundaries.cjs` CI step. The same gates cover feature-local components under
`src/features/*/components/**`.
- New code in `src/features/*/application/**` that imports React/JSX/DOM fails the same gates.
- A `use*.ts(x)` module placed at a feature's root directory fails the `check-boundaries.cjs`
CI step.
- A reviewer can point at this ADR instead of re-arguing the layering on every PR.
- The allowlist is technical debt with an expiration date: each entry is a component that still
needs its operations lifted into a `src/features/*/hooks/*` hook.

## Open issues / follow-up

- Migrate the allowlist tail components into hooks, one feature per PR, removing each from the
allowlist as it lands. This ADR does not schedule that work; it only forbids *new* direct
imports.
- `src/features/discovery/hooks/useDiscoveryRepoActions.ts` imports
`src/features/repositories/application/discoveryRepoPatches` — a sideways import into a
sibling feature's internals (forbidden by the Decision above, not yet enforced by any tool).
Either lift the shared patch into a neutral module or route it through the Store; not
scheduled here.
- If a future PR genuinely needs a component to call a business service (e.g. a throwaway debug
component), the answer is a new hook in the right feature, not an `eslint-disable` comment.
9 changes: 8 additions & 1 deletion eslint.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,16 @@ export default tseslint.config(
* `no-restricted-syntax` covers dynamic `import('...')` (ImportExpression), which
* no-restricted-imports does not inspect.
*/
files: ['src/components/**/*.{ts,tsx}'],
// Covers the shared View tier (src/components/**) and feature-local view
// components (src/features/*/components/**), which are View tier per ADR 0001
// and must not import business services either.
files: [
'src/components/**/*.{ts,tsx}',
'src/features/*/components/**/*.{ts,tsx}',
],
ignores: [
'src/components/**/*.test.{ts,tsx}',
'src/features/*/components/**/*.test.{ts,tsx}',
],
rules: {
'no-restricted-imports': [
Expand Down
30 changes: 26 additions & 4 deletions scripts/check-boundaries.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,20 @@
* A standalone, offline scanner that mirrors the ESLint `no-restricted-imports`
* rules in eslint.config.js. It runs in CI as a defense-in-depth check so that
* a misconfigured lint pass (or a contributor who disables the rule inline)
* cannot silently let a View component import a business service, or an
* application command import React/JSX/the Store/a service.
* cannot silently let a View component (shared or feature-local) import a
* business service, an application command import React/JSX/the Store/a service,
* or a feature hook sit outside its feature's hooks/ directory (or src/hooks/
* when shared across features).
*
* Contract source of truth: docs/adr/0001-frontend-layering.md
*
* Properties:
* - No network, no dynamic import, no execution of repo code. It only reads
* source files as text and pattern-matches import statements.
* - Exempts test files (*.test.ts / *.test.tsx) so vi.mock('../services/...')
* stays legal.
* - Import checks exempt test files (*.test.ts / *.test.tsx) so
* vi.mock('../services/...') stays legal. The hook-placement check applies
* to test files too: a use* test at a feature root means the code is
* misplaced regardless.
*
* Exit code: 0 = clean, 1 = violations found (or unreadable file).
*/
Expand Down Expand Up @@ -167,7 +171,25 @@ const files = walk(path.join(ROOT, 'src'));
for (const full of files) {
const relPath = rel(full);
if (relPath.startsWith('src/components/')) checkComponentFile(full, relPath);
// Feature-local view components are View tier too (ADR 0001): same service ban.
else if (/^src\/features\/[^/]+\/components\//.test(relPath)) checkComponentFile(full, relPath);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
else if (/^src\/features\/[^/]+\/application\//.test(relPath)) checkApplicationFile(full, relPath);
else if (/^src\/features\/[^/]+\/[^/]+$/.test(relPath)) checkFeatureRootFile(relPath);
}

/**
* ADR 0001: domain hooks live in src/features/<feature>/hooks/**; hooks shared
* across features live in src/hooks/**. A use* module sitting directly at a
* feature's root is a placement violation — historically it also hid cross-feature
* hook imports (ADR footnote 2) from every automated check.
*/
function checkFeatureRootFile(relPath) {
const basename = relPath.split('/').pop();
if (!/^use[A-Z].*\.tsx?$/.test(basename)) return;
console.error(
`✖ ${relPath}: feature hooks must live in src/features/*/hooks/** (or src/hooks/** when shared across features). See docs/adr/0001-frontend-layering.md.`,
);
violations++;
}

if (violations > 0) {
Expand Down
2 changes: 1 addition & 1 deletion src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { useAutoUpdateCheck } from './hooks/useAutoUpdateCheck';
import { logger } from './services/logger';
import { UpdateNotificationBanner } from './components/UpdateNotificationBanner';
import { ListsPushIndicator } from './components/ListsPushIndicator';
import { useBackendLifecycle } from './features/lifecycle/useBackendLifecycle';
import { useBackendLifecycle } from './features/lifecycle/hooks/useBackendLifecycle';
import type { AppState } from './types';
import { hasActiveSearchFilters } from './utils/repoSearch';

Expand Down
14 changes: 7 additions & 7 deletions src/components/GlobalChatHistorySheet.test.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { fireEvent, render, screen, within } from '@testing-library/react';
import { beforeEach, describe, expect, it, vi } from 'vitest';
import { GlobalChatHistorySheet } from './GlobalChatHistorySheet';
import { repositoryChatSessionRepository } from '../features/repository-chat/repositories/sessionRepository';
import { repositoryChatStorage } from '../services/repositoryChatStorage';
import type { Repository } from '../types';
import type { RepositoryChatSession } from '../types/repositoryChat';

Expand Down Expand Up @@ -35,8 +35,8 @@ describe('GlobalChatHistorySheet', () => {
});

it('按更新时间倒序列出跨仓会话', async () => {
await repositoryChatSessionRepository.saveSession(createSession('older', 1, 'owner/repo-one', '2026-08-24T00:00:00.000Z'));
await repositoryChatSessionRepository.saveSession(createSession('newer', 2, 'owner/repo-two', '2026-08-26T00:00:00.000Z'));
await repositoryChatStorage.saveSession(createSession('older', 1, 'owner/repo-one', '2026-08-24T00:00:00.000Z'));
await repositoryChatStorage.saveSession(createSession('newer', 2, 'owner/repo-two', '2026-08-26T00:00:00.000Z'));

render(<GlobalChatHistorySheet isOpen repositories={repositories} onClose={() => {}} onSelectSession={() => {}} />);

Expand All @@ -48,8 +48,8 @@ describe('GlobalChatHistorySheet', () => {
});

it('按标题或仓库名过滤并进入对应仓库会话', async () => {
await repositoryChatSessionRepository.saveSession(createSession('s1', 1, 'owner/repo-one', '2026-08-24T00:00:00.000Z'));
await repositoryChatSessionRepository.saveSession(createSession('s2', 2, 'owner/repo-two', '2026-08-26T00:00:00.000Z'));
await repositoryChatStorage.saveSession(createSession('s1', 1, 'owner/repo-one', '2026-08-24T00:00:00.000Z'));
await repositoryChatStorage.saveSession(createSession('s2', 2, 'owner/repo-two', '2026-08-26T00:00:00.000Z'));
const onSelectSession = vi.fn();

render(<GlobalChatHistorySheet isOpen repositories={repositories} onClose={() => {}} onSelectSession={onSelectSession} />);
Expand All @@ -62,7 +62,7 @@ describe('GlobalChatHistorySheet', () => {
expect(onSelectSession).toHaveBeenCalledWith(repositories[1], 's2');
});

it('删除会话后从列表移除', async () => { await repositoryChatSessionRepository.saveSession(createSession('doomed', 1, 'owner/repo-one', '2026-08-26T00:00:00.000Z'));
it('删除会话后从列表移除', async () => { await repositoryChatStorage.saveSession(createSession('doomed', 1, 'owner/repo-one', '2026-08-26T00:00:00.000Z'));

render(<GlobalChatHistorySheet isOpen repositories={repositories} onClose={() => {}} onSelectSession={() => {}} />);
await screen.findByTitle('进入 owner/repo-one 的会话');
Expand All @@ -75,7 +75,7 @@ describe('GlobalChatHistorySheet', () => {
});

it('读取失败时显示错误与重试,恢复后可重载', async () => {
const listSpy = vi.spyOn(repositoryChatSessionRepository, 'listRecentSessions');
const listSpy = vi.spyOn(repositoryChatStorage, 'listRecentSessions');
listSpy.mockRejectedValueOnce(new Error('IndexedDB unavailable'));

render(<GlobalChatHistorySheet isOpen repositories={repositories} onClose={() => {}} onSelectSession={() => {}} />);
Expand Down
6 changes: 3 additions & 3 deletions src/components/GlobalChatHistorySheet.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { History, Search, Trash2 } from 'lucide-react';
import type { Repository } from '../types';
import type { RepositoryChatSession } from '../types/repositoryChat';
import { useAppStore } from '../store/useAppStore';
import { repositoryChatSessionRepository } from '../features/repository-chat/repositories/sessionRepository';
import { repositoryChatStorage } from '../services/repositoryChatStorage';
import { Button } from './ui/button';
import { Input } from './ui/input';
import { Sheet, SheetContent, SheetDescription, SheetHeader, SheetTitle } from './ui/sheet';
Expand Down Expand Up @@ -54,7 +54,7 @@ export const GlobalChatHistorySheet: React.FC<GlobalChatHistorySheetProps> = ({
setIsLoading(true);
setLoadError(null);
try {
const nextSessions = await repositoryChatSessionRepository.listRecentSessions(50);
const nextSessions = await repositoryChatStorage.listRecentSessions(50);
if (requestId !== requestIdRef.current) return;
setSessions(nextSessions);
} catch {
Expand All @@ -73,7 +73,7 @@ export const GlobalChatHistorySheet: React.FC<GlobalChatHistorySheetProps> = ({
}, [isOpen, refresh]);

const handleDelete = useCallback(async (sessionId: string) => {
await repositoryChatSessionRepository.permanentlyDeleteSession(sessionId);
await repositoryChatStorage.permanentlyDeleteSession(sessionId);
setSessions((previous) => previous.filter((session) => session.id !== sessionId));
window.dispatchEvent(new CustomEvent(HISTORY_CHANGE_EVENT));
}, []);
Expand Down
4 changes: 2 additions & 2 deletions src/components/ReleaseSourceSettingsModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type { CustomReleaseRepository, ReleaseSourceId } from '../types';
import { useAppStore } from '../store/useAppStore';
import { Modal } from './Modal';
import { useDialog } from '../hooks/useDialog';
import { useReleaseTimelineActions } from '../features/releases/hooks/useReleaseTimelineActions';
import { useWatchedSourcesSync } from '../features/releases/hooks/useWatchedSourcesSync';
import {
CUSTOM_RELEASE_SOURCE_ID,
RELEASE_SOURCE_LABELS,
Expand Down Expand Up @@ -211,7 +211,7 @@ interface WatchCustomReleaseSyncPanelProps {
const WatchCustomReleaseSyncPanel: React.FC<WatchCustomReleaseSyncPanelProps> = ({ repos, language }) => {
const githubToken = useAppStore(state => state.githubToken);
const updateReleaseSourceRepository = useAppStore(state => state.updateReleaseSourceRepository);
const { syncWatchedSources, isSyncingWatchedSources } = useReleaseTimelineActions();
const { syncWatchedSources, isSyncingWatchedSources } = useWatchedSourcesSync();
const isSyncing = isSyncingWatchedSources;

const t = (zh: string, en: string) => language === 'zh' ? zh : en;
Expand Down
4 changes: 2 additions & 2 deletions src/components/SearchBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { useSearchShortcuts } from '../hooks/useSearchShortcuts';
import { useSearchActions } from '../features/repositories/hooks/useSearchActions';
import { useDialog } from '../hooks/useDialog';
import { isRepoCustomized } from '../utils/repoUtils';
import { repositoryChatSessionRepository } from '../features/repository-chat/repositories/sessionRepository';
import { repositoryChatStorage } from '../services/repositoryChatStorage';
import { applyRepoFilters, performBasicTextSearch as basicTextSearch, sortRepositories } from '../utils/repoSearch';
import { NO_LICENSE_SENTINEL, normalizeLicense } from '../utils/licenseFilter';
import { NumberInput } from './ui/NumberInput';
Expand Down Expand Up @@ -125,7 +125,7 @@ export const SearchBar: React.FC = () => {
const refreshGlobalHistoryCount = async () => {
const requestId = ++globalHistoryRequestRef.current;
try {
const sessions = await repositoryChatSessionRepository.listRecentSessions(100);
const sessions = await repositoryChatStorage.listRecentSessions(100);
if (!cancelled && requestId === globalHistoryRequestRef.current) setGlobalHistoryCount(sessions.length);
} catch {
if (!cancelled && requestId === globalHistoryRequestRef.current) setGlobalHistoryCount(0);
Expand Down
Loading