107 lines
17 KiB
Markdown
107 lines
17 KiB
Markdown
# Project Review — 2026-10-24 (1)
|
||
|
||
## Вывод и область проверки
|
||
|
||
Модульный монолит, contract-first API и общий core соответствуют масштабу проекта. После предыдущего рефакторинга у library/uploads/player появились полезные границы. Перестраивать проект целиком ради SOLID оснований нет. Приоритет — изоляция асинхронных операций при смене аккаунта, авторизация записей и согласованность состояния рекомендаций.
|
||
|
||
Проверены карта документации и архитектура, локальные AGENTS, выбранные потоки auth, playback/events, wave, social/listening, uploads/placement, library/schedule, web persistence/dialogs, Android offline/events/downloads/onboarding, desktop proxy, Python wave/API и текущий diff. Это статическое ревью перечисленных потоков с целевыми тестами, а не построчная проверка каждого файла или полный security-аудит. Физический Android, реальные аудиопотоки, NFS и производительность под нагрузкой не проверялись. Замечания ниже подтверждены чтением кода; отдельные новые воспроизводящие тесты для них в этой работе не добавлялись.
|
||
|
||
## Prior Reviews Summary
|
||
|
||
В каталоге были два отчёта: `2026-10-22-review-1.md` и `2026-10-21-review-1.md`. Все 17 задач отмечены `[x]`; закрытые задачи повторно не открывались.
|
||
|
||
### Still Open (carried forward)
|
||
|
||
Нет. Перенесено: 0.
|
||
|
||
### Resolved Since Last Review
|
||
|
||
Нет ранее открытых задач для перепроверки. Подтверждено новых закрытий: 0.
|
||
|
||
## Phase 1: Code Quality
|
||
|
||
### SOLID
|
||
|
||
**F7, P3 — WaveService собирает инфраструктуру и выполняет сценарий.** Конструктор сам создаёт IndexHolder, SessionStore и RankerHolder, `batch` открывает PostgreSQL-соединения, загружает профиль, меняет Redis-сессию, вызывает алгоритм и пишет обучающие признаки. Изолированная проверка сценария требует подмены конкретных инфраструктурных деталей. Передавать узкие порты каталога/профиля, сессии и impression sink из `_serve`, оставив SQL/Redis в адаптерах. Не нужно оборачивать каждую математическую функцию интерфейсом. Источник: `services/recommender/pipu_rec/wave/service.py:58`, `:79`, `:112`; composition root: `pipu_rec/cli.py:_serve`.
|
||
|
||
Доказанных нарушений LSP в просмотренных реализациях не найдено. Остаточная длина Player и наличие конкретных адаптеров в composition root сами по себе не считаются дефектами. Закрытые замечания предыдущего SOLID-ревью не дублируются.
|
||
|
||
### Performance
|
||
|
||
**F5, P2 — размер исключений растёт без ограничения и нарушает внутренний контракт.** Go `servedIDs` загружает все показы сессии и передаёт весь массив как `exclude_ids`, а Python BatchRequest ограничивает его 1000 элементами. После превышения 1000 показов запросы этой сессии получают 422, и Go каждый раз переходит на fallback. Это подтверждённый конфликт границ, а не результат нагрузочного замера. Согласовать bounded history/хранение исключений на стороне рекомендателя, сохранив политику неповторов. Источники: `services/api/internal/wave/wave.go:291`, `:349`; `services/recommender/pipu_rec/wave/service.py:40`.
|
||
|
||
Иных измеренных проблем производительности не установлено.
|
||
|
||
### Correctness & Bugs
|
||
|
||
**F1, P1 — незавершённая загрузка Android продолжает работу в новом аккаунте.** `attach` меняет `this.owner/server` и очищает active; `run` захватывает старую session, но после сетевого await строит путь через изменяемый `this.file` и URL через `this.abs`, затем без проверки поколения снова вставляет DownloadResumable в active. Сценарий: медленный `/tracks/batch` → logout/смена сервера → старый ответ. Старая задача может писать в каталог нового владельца и занимать его очередь; её finally может удалить active новой задачи с тем же track ID. Проверка owner внутри `status` не защищает создание загрузки, progress callback и файловые операции. Захватывать контекст задачи (owner/server/пути), проверять поколение после await и удалять active только по идентичности задачи. Источник: `apps/mobile/src/offline/downloads.ts:84`, `:228`, `:235`, `:261`.
|
||
|
||
**F3, P2 — поздний refresh восстанавливает устаревшую авторизацию.** `doRefresh` без проверки версии вызывает `apply`; logout/signOut и новый login не инвалидируют уже отправленный refresh. Если успешный refresh был обработан сервером до logout, но его ответ задержался, он может после signOut вернуть локальный signed_in и записать старые токены в RefreshStore. Аналогично ответ старого refresh может перезаписать новый login. Нужны поколение auth-операций и отбрасывание устаревшего результата до изменения access/store/state. Источник: `packages/core/src/auth.ts:143`, `:159`, `:167`, `:197`.
|
||
|
||
### Code Quality
|
||
|
||
Отдельных задач по косметике, размеру файлов и magic numbers не создано: они уступают проверенным нарушениям поведения.
|
||
|
||
## Phase 2: Logical Consistency
|
||
|
||
### Domain & Application Layer
|
||
|
||
**F2, P1 — событие пользователя может изменить исход чужой wave-сессии.** Клиент передаёт произвольный UUID в context_id; Accept передаёт его в MarkImpression, где UPDATE ограничен только sessionID/trackID. Проверки `wave_sessions.user_id = p.UserID` нет. Зная UUID чужой сессии и трека, авторизованный клиент может записать like/dislike/skip в чужой показ, влияя на метрики и обучающие данные. UUID усложняет угадывание, но не заменяет авторизацию. Передавать userID и ограничивать запись владельцем сессии в той же SQL-операции; добавить тест изоляции двух пользователей. Источники: `services/api/internal/playback/events.go:75`, `services/api/internal/playback/store.go:354`; контракт `api/openapi.yaml:PlayEvent` разрешает context_id как строку.
|
||
|
||
### Data Flow
|
||
|
||
Сохранены положительные границы: core не импортирует платформенные реализации, uploads use cases отделены от pgx, read-проекции могут делать межмодульные JOIN. Целевая проверка `pnpm check:boundaries` проходит; это не доказательство отсутствия всех циклов импортов.
|
||
|
||
### State Management
|
||
|
||
**F4, P2 — drain реакций может перезаписать более свежий выбор.** `drain` читает snapshot pending_reactions и отправляет его без координации с `react`. Возможен порядок: drain прочитал старый like → react успешно записал новый dislike и удалил pending → drain позже отправил старый like. Условие `reaction IS ?` защищает удаление SQLite-строки, но не порядок запросов к серверу. Сериализовать операции по track ID либо добавить версию и согласовать запись на сервере. Источник: `apps/mobile/src/offline/events.ts:50`, `:65`, `:77`, `:82`.
|
||
|
||
**F6, P2 — параллельные запросы теряют обновления Redis-сессии волны.** API запускает batch через `asyncio.to_thread`; каждый batch отдельно делает get → меняет session → put. Два запроса одной сессии могут прочитать одинаковый snapshot, выбрать пересекающиеся треки и последней записью затереть feedback/served/recent_artists первого. PostgreSQL-обновление tracks_served не защищает Redis и выбор кандидатов. Нужна сериализация/версионирование всей операции по session_id с учётом нескольких процессов, а не только локальный thread lock. Источники: `services/recommender/pipu_rec/api/app.py:wave_batch`, `pipu_rec/wave/service.py:84`, `:106`, `pipu_rec/wave/store.py:SessionStore`.
|
||
|
||
### Consistency
|
||
|
||
Конфликт внутреннего контракта Go/Python описан как F5; отдельная дублирующая задача не создаётся.
|
||
|
||
## Phase 3: UI/UX
|
||
|
||
### Usability
|
||
|
||
**F8, P2 — Android молча пропускает onboarding при ошибке его загрузки.** После завершения запроса с ошибкой `query.isPending` становится false, data отсутствует, и `!query.data?.required` возвращает основной интерфейс без ошибки и retry. Для нового пользователя временный сбой выглядит как отсутствие настройки рекомендаций. Разделить unknown/error и подтверждённое required=false; предусмотреть retry и осознанный вход в offline-режим без блокирования скачанной музыки. Источник: `apps/mobile/src/ui/Onboarding.tsx:29`.
|
||
|
||
### Visual & Consistency
|
||
|
||
Новых подтверждённых визуальных замечаний нет. Старые закрытые mobile-web задачи не переоткрывались. В этой работе браузер использовался для функциональных E2E; полного визуального аудита всех экранов не было.
|
||
|
||
### Interaction & Feedback
|
||
|
||
В текущих висящих изменениях завершён lifecycle listening: закрытие диалога после отправки, отсутствие стрима до admission, отдельная ошибка listening, явный запуск на втором устройстве ведущего, защита от позднего GET после выхода и сохранение refresh во время запроса. Дополнительно исправлено освобождение пустой скрытой комнаты после отмены/истечения последнего запроса; музыка источника продолжает играть. Эти исправленные в рамках сдачи дефекты не входят в открытые Tasks.
|
||
|
||
### Accessibility
|
||
|
||
У просмотренного web Dialog есть именование, focus trap, Escape и возврат фокуса; у Android Field/Button есть доступные имена/роли. Новых доказанных accessibility-дефектов не заявлено. Проверка screen reader и всех контрастов не проводилась.
|
||
|
||
## Tasks
|
||
|
||
Новых открытых задач: 8. Перенесено: 0. Ранее открытых, исправленных после прошлых ревью: 0. Исправление всех найденных замечаний не входило в просьбу завершить уже висящие изменения listening.
|
||
|
||
- [ ] 1. [Bug] [P1, F1] Изолировать Download jobs по поколению owner/server, включая URL, пути, progress и active cleanup — `apps/mobile/src/offline/downloads.ts` line 261.
|
||
- [ ] 2. [Logic] [P1, F2] Проверять владельца wave-сессии при MarkImpression и покрыть изоляцию пользователей — `services/api/internal/playback/store.go` line 354.
|
||
- [ ] 3. [Bug] [P2, F3] Инвалидировать поздние refresh/apply после logout и нового login — `packages/core/src/auth.ts` line 197.
|
||
- [ ] 4. [Bug] [P2, F4] Согласовать порядок drain и новых реакций по track ID — `apps/mobile/src/offline/events.ts` line 82.
|
||
- [ ] 5. [Performance] [P2, F5] Согласовать bounded exclude history Go/Python, исключить постоянный fallback после 1000 показов — `services/api/internal/wave/wave.go` line 349.
|
||
- [ ] 6. [Logic] [P2, F6] Защитить read-modify-write wave-сессии от параллельных batch — `services/recommender/pipu_rec/wave/service.py` line 84.
|
||
- [ ] 7. [UX] [P2, F8] Отделить ошибку onboarding от required=false и дать retry/offline-путь — `apps/mobile/src/ui/Onboarding.tsx` line 29.
|
||
- [ ] 8. [SOLID] [P3, F7] Передавать узкие IO-порты в WaveService из composition root — `services/recommender/pipu_rec/wave/service.py` line 58.
|
||
|
||
## Проверки и Git
|
||
|
||
- До изменений этого ревью: HEAD `cb22f15`; после успешного `git fetch origin` — HEAD и origin/main совпадали, ahead=0, behind=0.
|
||
- В рабочем дереве изначально: 13 изменённых tracked-файлов listening/тестов/переводов/docs, новый `apps/web/e2e/listening.spec.ts`, а также отдельные `scripts/share-stats.py`, `artifacts/wave-palette/`, `services/api/api.exe`. Последние три не менялись и не включаются в коммит listening.
|
||
- Core: 69 тестов прошли, в том числе новый тест освобождения пустой комнаты.
|
||
- Web: App + player/listening — 51 тест прошёл в итоговом прогоне. В первом запуске при параллельных проверках упал lossless-hint test; изолированно и в повторном прогоне проходит. Стабильность этого теста не считается доказанной одним повтором.
|
||
- Mobile: 6 тестов прошли. Web/mobile/core typecheck и i18n check прошли (1111 ключей × 2 языка).
|
||
- `pnpm check:boundaries` прошёл. `git diff --check` прошёл.
|
||
- Go: unit-тесты social/playback/wave и vet social прошли. Integration `TestActivityRoomsAndChatPrivacy` собирается, но **SKIP**: TEST_DATABASE_URL не задан. Docker daemon недоступен. Транзакционные сценарии на реальной БД этой проверкой не подтверждены.
|
||
- Playwright listening: 4 сценария прошли. Используется fake API; ошибки аудиопрокси ECONNREFUSED в сценариях не являются проверкой воспроизведения настоящего аудио.
|
||
- Публичный OpenAPI не менялся, генерация не требуется. Реальный Android/background и PostgreSQL остаются ограничениями проверки.
|