# WP2 report — the products screen's other nine filter defects **Status:** DONE **Branch:** `hazemdev2` · **Commit:** `865ba0983` (not pushed, not merged) **Build:** `npx ng build --base-href /app/` green · `npx tsc --noEmit` exit 0, no new errors **Backend:** untouched. ## Files changed (7) | File | Δ | Why | |---|---|---| | `src/app/features/products/products.component.ts` | +288/−90 | D1, D2, D3, D4, D5, D7, D8 | | `src/app/features/products/products.component.html` | +20/−7 | D4, D5, D6, and the `displayProducts()` removal | | `src/app/features/products/products.component.scss` | +17 | the tree-view filter note's style | | `src/app/core/services/product.service.ts` | +8/−4 | D8 in the serializer | | `src/app/shared/components/data-table/data-table.component.ts` | +4/−2 | D5 — the export buttons' label | | `src/assets/i18n/ar.json`, `src/assets/i18n/en.json` | +4 keys each | 4 new keys, additive, both files | No `git checkout` / `restore` / `stash` was run on anything, i18n files included. --- ## Method — how the claims below are evidenced No test runner in this repo (`skipTests: true`). I used WP1's technique, extended. **Harness A — the component's request pipeline.** A Node script reads `products.component.ts`, brace-matches the **real source text** of `initRequestPipeline`, `loadPage`, `onFilterSearch`, `onFilterChange`, `afterFilterChange`, `onPageChange` and `buildFilters` out of the file, pastes those method bodies verbatim into a class with stubbed collaborators, transpiles it with the repo's own esbuild (bundling the repo's own rxjs), and runs it. The statements executed are the production statements character for character; what is simulated is the caller (a keystroke, a click) and the transport (`productService.list` returns an `Observable` that emits after a chosen delay and logs its own subscribe / abort / emit). The same script was run unchanged against `git show HEAD:…products.component.ts` for the RED baseline. **Harness B — the service's query string.** WP1's harness, re-run: bundle the real `product.service.ts`, `Object.create(ProductService.prototype)` so the constructor's `inject(HttpClient)` never runs, stub `http.get` to capture `opts.params`, call the real `list()` body, print `HttpParams.toString()`. Both harness files live only in the session scratchpad; the temporary `.ts` entry points written into the repo root were deleted, and `git status` is clean apart from the commit. **Not claimed:** nobody clicked the live screen and no request left a browser. Everything below is "the code does X", verified by running the code — not "the server answered Y". --- ## The nine ### D1 — search fired one request per keystroke, with no ordering guarantee ✅ *(both halves)* Two separate mechanisms, because they solve two separate problems. **Half 1 — debounce.** `onFilterSearch()` now pushes the raw value onto `filterSearchInput`, a `Subject` piped through `debounceTime(400)` + `distinctUntilChanged()`. 400 ms is the app's existing convention (`stock-balances.component.ts:220`). Only the *search box* is debounced — a page change or a select still calls `loadPage()` immediately, because delaying those would be a regression in feel, not a fix. **Half 2 — ordering.** Every products-list request now leaves through one `Subject` (`pageRequests`) whose pipeline is `switchMap`. `switchMap` **unsubscribes the in-flight request** before starting the next, which aborts the HTTP call — a late answer to an older query is not merely ignored, it never arrives. `catchError` sits *inside* the `switchMap` projection deliberately: hoisted outside it, the first failed request would complete the outer stream and the screen would stop answering filters altogether. `loadPage()` no longer subscribes; it snapshots `buildFilters()` and emits. Snapshotting at call time (not inside the pipeline) means a request always carries the filter state that triggered it. **Demonstration — three keystrokes 100 ms apart:** ``` RED (HEAD) GREEN (this commit) REQUEST search="a" REQUEST search="abc" REQUEST search="ab" ↳ RESPONSE lands: abc REQUEST search="abc" RESULT: 1 request for 3 keystrokes RESULT: 3 requests for 3 keystrokes ``` **Demonstration — out-of-order responses.** Query `old` is issued first and made to take 600 ms; query `new` is issued 20 ms later and takes 50 ms. This is exactly the case debouncing cannot fix (the two requests are 20 ms apart, but they could equally be 5 s apart — the point is that an *earlier* query answers *later*): ``` RED (HEAD) GREEN (this commit) REQUEST search="old" REQUEST search="old" REQUEST search="new" ↳ ABORTED (unsubscribed): old ↳ RESPONSE lands: new REQUEST search="new" ↳ RESPONSE lands: old ↳ RESPONSE lands: new RESULT: table shows "old" ← stale RESULT: table shows "new" ``` RED repaints the table with the stale result. GREEN aborts `old` before it can answer. `tableLoading` ends `false` in both. ### D2 — dead parallel search machinery ✅ deleted Removed: `searchQuery`, `searchResults`, `searchLoading`, `searchSubject`, the `ngOnInit` subscription that drove them, `onSearch()`, `clearSearch()`, and `displayProducts()` — including the `results !== null ? results : tableData()` fallback branch, which is the landmine: a non-null `searchResults()` replaced the filtered, paginated table with a list that `ProductService.search()` had built from `search` (+ optional `type`) alone, silently dropping category, status and toll customer. **Nothing still reads it.** Verified before deleting (template grep found no `searchQuery` / `onSearch(` / `clearSearch` / `searchResults`), and again after: a grep for all seven identifiers across `products.component.{ts,html,scss}` returns nothing. The two call sites of `displayProducts()` — `[data]` in the template and `openBatchPrint()` — now read `tableData()` directly, which is what they got in practice anyway. Also removed as collateral: the `minSearchChars` signal and its `settingService.getByKey('products.min_search_chars')` request, which existed **only** to gate the deleted path. That is one fewer HTTP request on screen open. The setting itself is untouched and still read by nine other screens (inventory-counts, both orders screens, stock-receipts, warehouse-transfers, purchases requests, opening-balance, commissions, sales orders) — nothing there changes. `ProductService.search()` is left in place. A repo-wide grep for `productService.search(` now returns **nothing** — it has no callers left anywhere. I did not delete it because the brief scoped D2 to the component's machinery, and removing a public service method (which also fans out into an all-pages `forkJoin`) is a different, service-layer decision. Flagged in Concerns instead. ### D3 — double initial load ✅ one request on open `onPageChange()` now opens with the `initialLoaded` guard, byte-for-byte the idiom at `stock-balances.component.ts:176-179`: the lazy ``'s init `onLazyLoad` is swallowed once, because `ngOnInit` has already loaded page 1. ``` RED: ngOnInit loadPage() + init onLazyLoad → 2 requests on open GREEN: same two calls → 1 request on open ``` ### D4 — tree view ignored every filter ✅ *(judgement call — both, see below)* **Decision: pass the filters through AND say so.** - `onTreeNodeExpand()` now fetches a category's products with `{ ...this.buildFilters(), product_category_id: node.data.id }` — search, type, status and toll customer all carry through; the category comes from the node and therefore overrides `product_category_id`. - A filter change now calls `resetTree()`, which drops the cached nodes (an already-expanded category was holding products fetched under the *old* filters — the same silent lie one level down) and reloads the tree if it is the visible mode. - The category **hierarchy** still comes from `productCategoryService.tree()`, which has no filter parameter. So when any filter is active the tree toolbar shows a pill: «الفلاتر مطبَّقة على المنتجات — شجرة التصنيفات تظهر كاملة» / "Filters apply to products — the category tree is shown in full". It renders only when `hasActiveFilters`, so an unfiltered tree looks exactly as it did. **Why both rather than one:** passing the filters through is where the user's question actually gets answered (they filtered by toll customer; they want that customer's items under each category), and it was cheap. But it would have been a *new* half-truth to stop there — the category list is still complete, and a user who filtered to one category would still see the whole tree. Filtering the category tree client-side would mean guessing which categories have matching products without asking the server, which is a bigger change than this WP's mandate. So the part that is filtered is filtered, and the part that is not says that it is not. The unacceptable outcome — showing everything silently — is gone either way. ### D5 — batch print / export covered only the visible page ✅ *(judgement call — labelled, not widened)* **Decision: make the scope truthful, do not widen it.** - The batch-print button's tooltip is now `PRODUCTS.BATCH_PRINT_CURRENT_PAGE` plus the live row count — "طباعة باركود منتجات الصفحة الحالية (25)". `openBatchPrint()` behaviour is unchanged. - The shared `app-data-table`'s Excel and PDF tooltips now read `COMMON.EXPORT_EXCEL_PAGE` / `COMMON.EXPORT_PDF_PAGE` — "تصدير Excel (الصفحة الحالية)" — **when and only when `lazy()` is true.** A non-lazy table holds all its rows, so its export really is everything and its label is unchanged. **Why not fetch all filtered rows:** the API caps a page at 25 (`moon-erp/CLAUDE.md`), so "print every filtered row" on a 4 000-item filter is 160 requests before the dialog opens, with no progress indication and no way to cancel — and a 4 000-label print run is not a thing anyone has asked for. Same arithmetic for export. If it is ever wanted it should be a separate, explicitly-confirmed action, not a silent widening of a button that already exists. Out of proportion here; left labelled, exactly as the brief permits. **Scope note:** this is the one edit outside the three briefed files. It is 2 tooltip bindings in the shared component, gated on `lazy()`, changing text only — no export behaviour anywhere changed. I judged that better than duplicating a truthful label onto one screen while every other lazy table keeps the misleading one; per the standing "fix in the shared layer" rule. ### D6 — `filterBy` searched fields the user cannot see ✅ - Toll-customer select: `filterBy="display_label,name,name_ar,material_code_prefix"` — `display_label` is what the option shows (`name_ar (PREFIX)`, built at `:1108-1114`), and it now leads the list. - Category select: `filterBy="display_name,name,name_ar,name_en"` — `display_name` is the `Parent › Child` path the option shows. Typing a parent's name found nothing before. Both keep their previous fields, so nothing that used to match stops matching. ### D7 — no filter persistence ✅ URL-synced All five filters now round-trip through the query string as `q`, `type`, `status`, `cat`, `customer`. `syncFiltersToUrl()` follows the `stock-balances.component.ts:263-267` precedent (`queryParamsHandling: 'merge'`, `null` to drop a key) with one addition: `replaceUrl: true`, because the search box writes on every typing pause and each pause must not become a browser-history entry. `readFiltersFromUrl()` runs once in `ngOnInit`, before the first `loadPage()`. `customer` is parsed as *either* a numeric partner id *or* the literal `'own'`, so WP1's sentinel survives a refresh. One consequence had to be handled: the `viewId` deep-link handler subscribes to `queryParams`, which now re-emits on every filter change. Without a guard the product view dialog would have re-opened on every keystroke pause. It is now guarded on `handledViewId` and fires once per id. That subscription is also now pushed onto `subscriptions` — it was leaking before. ### D8 — truthiness guards ✅ converted to `!= null` **Decision: convert, in both layers.** Truthiness reads as "a filter was chosen" but actually means "the value is not `0 / '' / null / undefined`". For a category id that is a live hazard the moment an id can be `0`, and it is the exact idiom `stock-balance.service.ts:125` avoids on purpose (there the "own" sentinel *is* `0`, so truthiness would be an outright bug). WP1 already used `!= null` for `toll_customer_id`; leaving its five neighbours on truthiness would have made that look like a local quirk rather than the rule. `buildFilters()` also now **trims** the search term — a box holding only spaces is not a filter — and `hasActiveFilters` is derived from `buildFilters()` instead of repeating the guard list, so the pill and the request can no longer disagree. `ProductService.list()`'s `search` / `type` / `status` / `product_category_id` got the same treatment. Harness B, real `HttpParams` out of the real `list()`: ``` no filters → page=1&per_page=100 category 0 → page=1&per_page=100&product_category_id=0 ← was dropped before category 3 → page=1&per_page=100&product_category_id=3 empty strings → page=1&per_page=100 ← no `search=` sent ``` And `buildFilters()` itself (harness A, real method body): | state | RED | GREEN | |---|---|---| | all cleared | `{}` | `{}` | | category id `0` | `{}` ← dropped | `{"product_category_id":0}` | | search `" "` | `{"search":" "}` | `{}` | | search `" abc "` | `{"search":" abc "}` | `{"search":"abc"}` | | customer `'own'` | `{"toll_customer_id":"own"}` | same | | customer `42` | `{"toll_customer_id":42}` | same | | type `""`, status `""` | `{}` | `{}` | ### D9 — `brand_id` ✅ reported only, not added `ProductService::search()` on the backend applies `brand_id` (`:228-230`) and no frontend screen sends it. **Nothing was added** — the owner asked for the existing filters to work, not for a new one. Recording it here so it is not rediscovered as a bug: this is an unused server capability, and adding a brand select would be a small, self-contained follow-up if the owner ever wants it (a `brands` lookup exists; the filter bar has room). --- ## Acceptance criteria | # | Criterion | Verdict | |---|---|---| | 1 | One request per pause; a late response can never overwrite a newer one — **both demonstrated** | ✅ D1, RED/GREEN above | | 2 | One products request on open | ✅ D3, 2 → 1 | | 3 | Dead machinery gone; `displayProducts()` has no bypass branch | ✅ D2, method deleted entirely | | 4 | Tree view respects the filters or visibly says it does not | ✅ D4, both | | 5 | Both selects' type-to-filter matches what the option displays | ✅ D6 | | 6 | A filtered view survives refresh and is shareable as a URL | ✅ D7 | | 7 | WP1's customer filter still works | ✅ re-ran WP1's own harness — `toll_customer_id=42`, `toll_customer_id=own`, cleared → omitted, all unchanged; and `buildFilters()` still emits the key for both forms | | 8 | `ng build` green, `tsc --noEmit` no new errors | ✅ both, twice (once mid-way, once on the committed tree) | ## Housekeeping - `chown moonui2:moonui2` applied to all seven edited files. - **Not deployed to `/app`** — the orchestrator deploys. `dist/moon-erp/browser/` is current as of this commit. - Not pushed, not merged. Commit `865ba0983` on `hazemdev2`. - i18n: 4 keys added (`COMMON.EXPORT_EXCEL_PAGE`, `COMMON.EXPORT_PDF_PAGE`, `PRODUCTS.BATCH_PRINT_CURRENT_PAGE`, `PRODUCTS.TREE_FILTER_NOTE`), in **both** files, purely additive — the diff on each file is 4 added lines and 2 comma-only changes, nothing else. ## Concerns for the orchestrator 1. **Nothing here has been exercised against a live server.** The harnesses prove the code behaves as described; the first real click is still owed. Highest-value single check after deploy: type in the search box and watch the network tab show one request per pause, then reload the page and confirm the filters come back. 2. **One shared-component edit** (`data-table.component.ts`, D5). Text-only, gated on `lazy()`, but it does touch every screen that uses `app-data-table`. If the orchestrator wants WP2 confined to the three briefed files, reverting those 4 lines costs nothing and loses only the truthful export label. 3. **Release note not written.** The FE pre-push hook is warn-only and the product CHANGELOG lives in the backend repo (`moon-erp-be/docs/moonstack/CHANGELOG.md`), which this WP was told not to touch. This change is user-facing (search feel, shareable filtered links, honest button labels) and wants a bilingual `[Unreleased]` bullet before the next ship. 4. **Tree mode now refetches the category tree on every filter change** (`resetTree()`). That is one extra lightweight request per filter change, and only while the tree is the visible mode. It is the price of not showing stale children; flagged so it is not read later as an accidental N+1. 5. **`ProductService.search()` now has zero callers repo-wide** (verified by grep after the deletion) but is still exported. It is the method that silently drops three filters and fans out into an all-pages `forkJoin`. Deleting it is a one-line service-layer cleanup for whoever next touches `product.service.ts`; I left it because D2's mandate was the component's machinery, not the service's public surface.