diff --git a/docs/reviews/feed-tabs-summary.md b/docs/reviews/feed-tabs-summary.md new file mode 100644 index 0000000..5c2e083 --- /dev/null +++ b/docs/reviews/feed-tabs-summary.md @@ -0,0 +1,135 @@ +# Feed Tabs — Architect Review + +**By architect.** + +## Task and commits reviewed + +- Task: `feed-tabs` (refactorer handoff + `merge_and_process refactorer 4b8b4ed499`, delivered as a one-item `BATCH`). +- Merged `swarmforge-refactorer` (fast-forward from `de3f1c8`). Commits: + - `f3a41de` Specify merged posts feed with Recent/Following tabs (specifier) + - `d1372da` Merge Following feed into the posts page as Recent/Following tabs + (coder) + - `4b8b4ed` Refactor feed tabs: simplify mode loading, add feed-tabs + properties (refactorer) +- Architect review commit: `e4bda31256` (view/controller encapsulation + test + consolidation + tool-written mutation and acceptance manifests). +- Verification record `git_sha`: `e4bda31256` (client). +- Records/summary commit: this commit. + +The feature merges the old `/posts/following` route into `/posts/recent` as a +row of two mode buttons. On first open the page asks +`GET /follow/following/:addr`; if the viewer follows at least one account it +selects Following, otherwise Recent. Switching tabs resets to the first page. +The `/posts/following` route and navbar item are removed. The new +`FeedTabsPage` service composes the existing `RecentFeedPage` and +`FollowingFeedPage` controllers behind injected `memoDb`/`wallet`, and the React +shell (`components/app-body/posts/index.js`) renders it. Client-only read +feature; no DB or indexer change. + +## Architectural review + +- **UI/Core separation (good).** `FeedTabsPage` is pure coordinator logic in + `src/services/` with `memoDb` and `wallet` injected; it opens no network, DOM, + or wallet IO itself. The React shell constructs `MemoDb` and the controller and + maps the snapshot into view state. Acceptance drives the controller directly + through the fake memo-db, so core behavior is tested with no UI or IO. +- **Dependency rule (good).** The view depends on the service; the service + depends on the two page controllers and on the `MemoDb` adapter through its + constructor. No high-level module reaches around the injected adapter, and the + adapter does not reach back into the controller. +- **Information hiding (one fix).** The controller exposes a tested `getState()` + snapshot, but the view was still reading `page.posts`, `page.pagination`, + `page.mode`, `page.emptyBecauseNoFollows`, and `page.offset` directly. Routed + `showPage` through `getState()` so the controller is the single source of feed + state (see Fixes). +- **Local code quality (one fix).** The two default-mode unit tests were + structurally identical and DRY reported them (score 1.00). Consolidated into a + table-driven pair with a shared assertion helper. Added initial-state tests to + kill mutation survivors the refactorer left (see Fixes). +- **Delegation, not duplication (accepted).** `FeedTabsPage` owns paging + (`nextPage`/`previousPage`/`canLoadMore`) and delegates post loading to the two + sub-pages. It computes `emptyBecauseNoFollows` itself from `hasFollows`, which + is the correct merged-page semantics (the message shows only when the viewer + follows no one). `FollowingFeedPage` still exposes its own + `emptyBecauseNoFollows`, but that is the older `/posts/following` contract and + is now unused by the tabs flow; it is retained only because + `specs/following-feed.feature` still specifies it. Not changed here (specifier + owns retiring that feature); noting it as the one remaining conceptual overlap. +- **Module boundaries (good).** The deleted `following-feed` component left no + dangling imports; `FollowingFeedPage` remains as the internal delegate used by + `FeedTabsPage`, and `memo-db.js` gained a thin `getFollowing` adapter method + next to the existing `getFollowingFeed`. + +## Fixes applied + +- **View consumes the controller snapshot.** `posts/index.js` `showPage` now + reads `page.getState()` instead of five internal fields, keeping the + controller's representation hidden behind its public accessor. The injected + `memoDb` (needed for author-profile lookup) is still passed explicitly. +- **Killed four mutation survivors with initial-state tests.** The refactorer's + first mutation run left `constructor.offset = 0`, `constructor.hasFollows = + false`, `constructor.emptyBecauseNoFollows = false`, and the + `canLoadMore() ?? false` default surviving because no test observed an + unloaded controller. Added `constructor starts in a neutral, unloaded state` + and `canLoadMore is false before any page has been loaded`. Final mutation: + **22 killed, 0 survived, 0 uncovered.** +- **Removed a local test duplicate.** Extracted + `assertOnlySelectedFeedLoaded` and folded the two default-mode tests into one + table so DRY reports no candidate. + +No production behavior changes were needed. The controller structure, injected +dependencies, and boundaries were accepted. + +## Verification results + +Record (pinned to the review commit `e4bda31256`): + +| File | Component | +|------|-----------| +| `docs/reviews/feed-tabs-verification.json` | psf-memo-client | + +- **Language mutation** (`mutate-file.sh src/services/feed-tabs-page.js + --max-workers 8`, run twice: the differential pass selected 0 changed sites, + so the wrapper auto-reran with `--mutate-all`): **22 killed, 0 survived, + 0 uncovered** (22 total/covered, 0 uncovered). +- **DRY** (`dry4javascript`, scoped to the changed production files and the + changed unit/property tests): **no duplicate candidates found.** The first run + flagged one local duplicate in the unit test file (two default-mode tests, + score 1.00); removing it cleared the report. A broad whole-client DRY run was + not needed and, per standing precedent, its pre-existing step-handler and + test-setup boilerplate is not part of this task. +- **Cyclomatic complexity / CRAP** (`crap4javascript + src/services/feed-tabs-page.js`): maximum **CRAP 5.0** at 100% coverage + (`open` CC 5); `_isEmptyFollowing`/`_loadMode`/`_normalizeTab` CC 3; every + other function CC ≤ 2. All functions are below the 6.0 target. +- **Soft Gherkin acceptance mutation** (`gherkin-mutator --level soft + --workers 8`), `psf-memo-client/specs/feed-tabs.feature`: **42 total, + 0 killed, 42 survived, 0 errors.** Every survivor is an intrinsic equivalent: + a single-character case/character substitution of an example value + (`followed_text`, `followed_txid`, `followee`, `other`, `other_text`, + `other_txid`) used consistently on both the setup and assertion sides of its + scenario, so the scenario still passes. This is the documented behavior for + this read-only feature; no implementation gap is implied. `gherkin-mutator` + wrote an `acceptance-mutation-manifest` with `"scenarios":[]` (each scenario + has an equivalent survivor, so none is recorded) and no `# mutation-stamp`; + committed as tool-written. + +## Suite status + +`verify.sh client` **result: pass** at `git_sha e4bda31256`: + +- unit: **506 pass / 0 fail** +- property: **121 pass / 0 fail** +- acceptance: **all 37 generated suites passed** +- lint: ok +- build: ok + +## Handoffs + +- End-of-chain `git_handoff` to `specifier` (priority 50) to merge + `swarmforge-architect` into `master`, using this records/summary commit. +- `git_handoff` to `coder` and `refactorer` (priority 00) with the review commit + `e4bda31256` for follow-up review: the view now consumes `getState()`, and the + default-mode tests were consolidated with added initial-state coverage; the + rest is tool-written mutation and acceptance manifests. diff --git a/docs/reviews/feed-tabs-verification.json b/docs/reviews/feed-tabs-verification.json new file mode 100644 index 0000000..051e117 --- /dev/null +++ b/docs/reviews/feed-tabs-verification.json @@ -0,0 +1,46 @@ +{ + "schema_version": 1, + "task": "feed-tabs", + "component": "psf-memo-client", + "git_sha": "e4bda31256dc8cd125e1ab1442f4f1b19cc4acff", + "branch": "swarmforge-architect", + "timestamp": "2026-09-20T17:38:07.672Z", + "commands": [ + { + "name": "unit", + "command": "npm test", + "exit": 0, + "duration_ms": 6556, + "summary": "506 pass / 0 fail" + }, + { + "name": "property", + "command": "npm run test:property", + "exit": 0, + "duration_ms": 9620, + "summary": "121 pass / 0 fail" + }, + { + "name": "acceptance", + "command": "npm run test:acceptance", + "exit": 0, + "duration_ms": 14023, + "summary": "all 37 acceptance suites passed" + }, + { + "name": "lint", + "command": "npm run lint", + "exit": 0, + "duration_ms": 1736, + "summary": "ok" + }, + { + "name": "build", + "command": "npm run build", + "exit": 0, + "duration_ms": 48366, + "summary": "ok" + } + ], + "result": "pass" +}