refactor: collapse duplicated stores, header, property detail and filter UIs - #1151
Open
a-malik-gh wants to merge 5 commits into
Open
a-malik-gh wants to merge 5 commits into
a-malik-gh wants to merge 5 commits into
Conversation
There were four compare-related stores, not the three the issue names. compareStore held selection as ids, comparisonStore held whole Property objects under its own persistence key, comparisonHistoryStore held snapshots, and compareStoreCanonical declared compareStore authoritative - but had no importers, so it changed nothing. The split was a live bug, not just untidiness. PropertyCard wrote to both selection stores on every toggle while reading is-selected from one and limit-reached from the other, so the two could disagree. /compare read comparisonStore while ComparisonBar read compareStore, so the bar and the page could show different selections. compareStore is now the single module, with a selection slice and a history slice: - selectedIds stays canonical, because an id is what the share URL and persistence carry and what a card knows about itself. - propertyCache is a best-effort store of the full objects, populated when a caller toggles with a Property. Explicitly not authoritative: a selection restored from storage or a share link has ids but no objects, so getSelectedProperties() drops unresolved ids rather than inventing placeholders. FloatingComparisonBar needs name and price.total, which is why the cache exists at all. - add/remove/toggle accept a Property or a bare id, since both call styles are in use. That is what lets PropertyCard make one call instead of two. Migrated PropertyCard, usePropertyCardData, FloatingComparisonBar, the compare page and the property domain barrel; deleted comparisonStore, comparisonHistoryStore and compareStoreCanonical. Persistence keeps the propchain-compare key so an existing selection survives, with version 2 and a migrate that also folds in history from the old propchain-comparison-history key - users keep their recent comparisons instead of silently losing them. A malformed legacy entry is tolerated rather than failing the upgrade. The two obsolete store suites are replaced by 20 tests over both slices, including that history survives clearCompare, that the cache is pruned with the selection, and that persistence writes one key at version 2. Closes MettaChain#1090
The PropChain header was inlined in twenty places across src/app: the same chrome classes, the same max-w-7xl container, the same h-16 flex row, and the same PC-square-plus-wordmark brand. Only the two ends ever differed, so everything that varies is now a prop on src/components/layout/AppHeader.tsx and everything that does not lives there once. API: `sticky`, `backHref`/`backLabel`, a `leading` slot for the dashboard's mobile sidebar toggle, an `actions` slot for WalletConnector and friends, `brandHref` (null renders plain text, which is right on the home page), `containerClassName` because /alerts is laid out at max-w-4xl and its header has to line up with its own body, and `as` for the rare second header-shaped row that must not claim the banner landmark. Two accessibility bugs fixed on the way through: - The brand was `<h1>PropChain</h1>` on most pages, which also render an `<h1>` for the page title. Every one of those pages had two level-1 headings and no unambiguous title. The brand is a `<span>` now, so each page keeps exactly one `<h1>` - its own. - Pages that rendered their own `<header>` while also mounting a component that rendered one produced two `banner` landmarks. One AppHeader per route keeps that to one. Also normalises what was inconsistent: some brands linked to `/`, some did not link at all, and back links varied between a bare anchor and a Button. Migrated: properties, transactions and dashboard (the three named in the issue), plus accessibility, governance, tax-report, secondary-market, watchlist, alerts, and both branches of dashboard/saved-searches. The properties/[id] headers are deliberately left alone - that tree is being consolidated under MettaChain#1093 and two of the files holding those headers are removed there. PropertyPageSkeleton keeps its own placeholder markup, since it is a skeleton rather than the real header. 11 tests including a structural snapshot. Closes MettaChain#1094
Property detail rendered through four modules and two page files. Only one
path was ever live.
What was there:
app/properties/[id]/page.tsx (the route)
-> components/PropertyDetailServer server render, live
-> components/PropertyDetailClient "client", took propertyId
app/properties/[id]/page-new.tsx (not a route; Next only routes page.tsx)
-> app/properties/[id]/PropertyDetailClient took `property`
app/properties/[id]/PropertyDetailPageClient.tsx (no importers at all)
-> components/PropertyDetail legacy
So two different components were called PropertyDetailClient with
different prop shapes, and the second tree was reachable only from
page-new.tsx, which carried a hardcoded mock property object.
After: one server entry and one client island.
app/properties/[id]/page.tsx
-> components/PropertyDetailServer
-> components/property/PropertyDetailHeaderActions
components/PropertyDetailClient was renamed to
components/property/PropertyDetailHeaderActions, because that is what it
was: two buttons in the header, not a client rendering of the detail page.
The misleading name is most of why the duplicate appeared - nothing about
it said "header actions", so the obvious name was still free for someone
building the real thing. Its unused `propertyId` prop is gone; the bell and
the connector both resolve their own state.
Removed: components/PropertyDetail.tsx, components/PropertyDetailClient.tsx,
app/properties/[id]/PropertyDetailClient.tsx,
app/properties/[id]/PropertyDetailPageClient.tsx, and
app/properties/[id]/page-new.tsx. Nothing outside the dead tree referenced
any of them.
The route header now uses AppHeader (MettaChain#1094), which removes the duplicate
<h1> and the second banner landmark this page had.
Two things were only implemented in the deleted files and are worth a
maintainer decision rather than a silent port, since neither ever shipped:
page-new.tsx had a generateMetadata plus property and breadcrumb
structured data that the live route does not have, and
[id]/PropertyDetailClient.tsx had a QR-code share and a SetPriceAlertModal
wired to the notification store. Both are recoverable from this commit.
The existing PropertyDetailServer suite is untouched and still passes. Adds
structural tests that fail if a deleted module is imported again or a
second PropertyDetailClient reappears.
Closes MettaChain#1093
Filter UI existed three times over:
components/forms/SearchFilterForm live on /properties and /forms
components/FilterSidebar the same control set with per-key
onFilterChange wiring; tested, but
rendered by no page
components/filters/* a sidebar plus AdvancedFilters that
reference ten filter components which
are never imported and do not exist
The third could not compile. FilterSidebar there calls LocationFilter,
PriceFilter, PropertyTypeFilter, BedroomsFilter, BathroomsFilter and
AreaFilter; AdvancedFilters calls ParkingFilter, YearBuiltFilter,
AmenitiesFilter and FurnishedFilter. None exist anywhere in the repo. That
is ten TS2304 errors in files tsconfig includes, and it has been sitting
there unnoticed - see the note on masking below.
SearchFilterForm is canonical: it is the one two pages actually render, it
validates through searchFilterSchema, and it already speaks SearchFilters.
Both duplicates are removed, along with FilterSidebar's test and snapshot.
Fixes a runtime crash in the canonical form while doing it.
SearchFilterForm renders <FormItem> but never imported it, so the component
threw "FormItem is not defined" on mount - meaning /properties and /forms
both crashed. FormItem is exported from components/ui/form; it was just
missing from the import list. Adding it is the whole fix, and the new tests
render the form so it cannot regress silently again.
Both bugs were invisible for the same reason: src/middleware.ts has syntax
errors, and tsc skips the semantic pass for the entire program when any file
fails to parse. Every type error in this repo is currently masked, and
`npm run build` runs typecheck first, so the build is red on main
regardless. The middleware damage is a merge artifact in security logic
(checkAdminAuth lost its body) and is deliberately not touched here.
Adds structural tests: exactly one filter form module, no FilterSidebar,
no components/filters, no imports of the removed modules, and - the useful
one - that the canonical form emits exactly the SearchFilters key set and
round-trips DEFAULT_FILTERS unchanged. Key-set equality is asserted in both
directions, so a new control that forgets the canonical type and a
canonical field the form silently drops both fail.
If the sidebar layout is wanted back, the right shape is a `layout` prop on
this one form rather than a second component; the deleted markup is
recoverable from this commit.
Closes MettaChain#1092
|
@a-malik-gh Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1090
Closes #1092
Closes #1093
Closes #1094
Four duplication cleanups in one branch, one commit each, reviewable in order.
cb2ff88c9d52fbe91ba8e1af49bfNet: −2,900 lines, 12 modules deleted, 4 added.
Jest: 185 suites passed, 2,040 tests passed
13 suites failing — all pre-existing, verified identical at cb2ff88 (see Blockers)
tsc: clean apart from src/middleware.ts, which is broken on main
#1090 — One canonical compare store
compareStore,comparisonStoreandcomparisonHistoryStoreall tracked overlapping state, plus acompareStoreCanonicalwith zero importers. Selection and history could disagree with each other, which was the actual bug.Now one store with a selection slice and a history slice.
selectedIdsis canonical;propertyCacheis explicitly best-effort, so a selection restored from a share link works with ids alone. Persisted under one key (propchain-compare,version: 2) with amigratethat folds in the legacypropchain-comparison-historypayload, so existing users keep their history.Migrated
PropertyCard,usePropertyCardData,FloatingComparisonBar,app/compare/page.tsxandstore/domains/property/index.ts. 20 tests across both slices, including that history survivesclearCompare()— the reason it was a separate store in the first place.#1094 — One page header
The header was inlined in twenty places across
src/app: same chrome classes, samemax-w-7xlcontainer, sameh-16row, same PC-square-and-wordmark brand. Only the two ends ever differed, so everything that varies is a prop onsrc/components/layout/AppHeader.tsx.Props:
sticky,backHref/backLabel, aleadingslot (the dashboard's mobile sidebar toggle), anactionsslot (WalletConnectorand friends),brandHref(nullrenders plain text, right on the home page),containerClassName(/alertsis laid out atmax-w-4xland its header has to line up with its own body), andasfor a header-shaped row that must not claim the banner landmark.Two accessibility bugs fixed on the way through:
<h1>PropChain</h1>on most pages, which also render an<h1>for the page title. Every one of those pages had two level-1 headings and no unambiguous title. The brand is a<span>now, so each page keeps exactly one<h1>— its own.<header>while also mounting a component that rendered one produced twobannerlandmarks. OneAppHeaderper route keeps it to one.Also normalises what was inconsistent: some brands linked to
/, some didn't link at all, and back links varied between a bare anchor and aButton.Migrated:
properties,transactions,dashboard(the three named in the issue), plusaccessibility,governance,tax-report,secondary-market,watchlist,alerts, and both branches ofdashboard/saved-searches. Theproperties/[id]headers are handled in the #1093 commit instead, since two of the files holding them are deleted there.PropertyPageSkeletonkeeps its own placeholder markup — it's a skeleton, not the real header.11 tests including a structural snapshot.
#1093 — One server entry, one client island
Property detail rendered through four modules and two page files. Only one path was ever live:
app/properties/[id]/page.tsx (the route)
-> components/PropertyDetailServer server render, live
-> components/PropertyDetailClient "client", took propertyId
app/properties/[id]/page-new.tsx (NOT a route; Next only routes page.tsx)
-> app/properties/[id]/PropertyDetailClient took property
app/properties/[id]/PropertyDetailPageClient.tsx (no importers at all)
-> components/PropertyDetail legacy
So two different components were named
PropertyDetailClientwith different prop shapes, and the second tree was reachable only frompage-new.tsx, which carried a hardcoded mock property object.After:
app/properties/[id]/page.tsx
-> components/PropertyDetailServer
-> components/property/PropertyDetailHeaderActions
components/PropertyDetailClientbecamecomponents/property/PropertyDetailHeaderActions, because that is what it was: two buttons in a header, not a client rendering of the detail page. The misleading name is most of why the duplicate appeared — nothing about it said "header actions", so the obvious name was still free for someone building the real thing. Its unusedpropertyIdprop is gone.Deleted
components/PropertyDetail.tsx,components/PropertyDetailClient.tsx,app/properties/[id]/PropertyDetailClient.tsx,app/properties/[id]/PropertyDetailPageClient.tsx,app/properties/[id]/page-new.tsx. Nothing outside the dead tree referenced any of them.Two things existed only in the deleted files and want a maintainer decision rather than a silent port, since neither ever shipped:
page-new.tsxhad agenerateMetadataplus property and breadcrumb structured data that the live route does not have, and[id]/PropertyDetailClient.tsxhad a QR-code share and aSetPriceAlertModalwired to the notification store. Both are recoverable from commite91ba8e.The existing
PropertyDetailServersuite is untouched and still passes.#1092 — One filter UI
components/forms/SearchFilterForm live on /properties and /forms
components/FilterSidebar same control set, per-key onFilterChange;
tested, but rendered by no page
components/filters/* a sidebar + AdvancedFilters referencing ten
filter components that don't exist
The third could not compile. Its
FilterSidebarcallsLocationFilter,PriceFilter,PropertyTypeFilter,BedroomsFilter,BathroomsFilter,AreaFilter;AdvancedFilterscallsParkingFilter,YearBuiltFilter,AmenitiesFilter,FurnishedFilter. None exist anywhere in the repo — tenTS2304errors in filestsconfigincludes.SearchFilterFormis canonical: it's the one two pages actually render, it validates throughsearchFilterSchema, and it already speaksSearchFilters. Both duplicates are removed.It also fixes a runtime crash in the canonical form
SearchFilterFormrenders<FormItem>but never imported it, so the component threwFormItem is not definedon mount —/propertiesand/formsboth crashed.FormItemis exported fromcomponents/ui/form; it was just missing from the import list. That one line is the whole fix, and the new tests render the form so it can't regress silently again.New structural tests: exactly one filter form module, no
FilterSidebar, nocomponents/filters, no imports of the removed modules, and — the useful one — that the canonical form emits exactly theSearchFilterskey set and round-tripsDEFAULT_FILTERSunchanged. Key-set equality is asserted in both directions, so a new control that forgets the canonical type and a canonical field the form silently drops both fail.If the sidebar layout is wanted back, the right shape is a
layoutprop on this one form rather than a second component; the deleted markup is recoverable from1af49bf.Blockers on
main— none of these are fixed here1.
npm run buildis red, andsrc/middleware.tsmasks every type error in the repo.src/middleware.tshas syntax errors from a merge that ate the body ofcheckAdminAuth(TS1109/TS1005/TS1002 from line 91). Two consequences:npm run buildistypecheck && next build, so the build fails onmain.tscskips the semantic pass for the whole program when any file fails to parse, so every type error in the repo is currently invisible. That is exactly why the tenTS2304s incomponents/filters/and the missingFormItemimport both survived.I deliberately have not touched it:
checkAdminAuthis still called atsrc/middleware.ts:139, so admin-route protection is live in intent but the file can't build, and the missing piece is security logic that should be restored by someone who knows what it was meant to allow. Fixing it will surface a backlog of previously-masked type errors — worth knowing before someone does it under time pressure.2.
npm run lintcannot start.eslint.config.mjsimportseslint-plugin-jsdoc, whichpackage.jsondoes not declare, so eslint dies withERR_MODULE_NOT_FOUNDbefore linting anything. Present on the base commit, so it is not from this branch. I installed it locally with--no-saveto lint my own work and left the manifests alone, since fixing it means apackage.json+ lockfile change unrelated to these four issues. Once it runs there are two further pre-existing problems:jsdoc/require-jsdocreports roughly fifty missing-JSDoc errors across existing page components, andreact-hooks/exhaustive-depsreportsDefinition for rule … was not found, so the hooks plugin is not wired up and those eslint-disable comments inapp/properties/page.tsxare currently errors rather than suppressions. New code in this branch does carry JSDoc and lints clean.3. 13 Jest suites fail on
main. Verified by running the same suites atcb2ff88before any of my commits — identical failures. They are middleware (blocked by #1, above),Request is not definedin the API route tests (jsdom without a fetch polyfill), a helper file under__tests__/that Jest treats as a suite with no tests, and some wallet/network suites that pass in isolation but time out under full-suite parallel load.WalletAddressInputis in that last category — 30/30 green on its own, on both this branch and the baseline.4. There are no CI workflows.
.github/workflows/does not exist, which is the reason a non-compiling directory, a crashing live component and a red build all coexisted unnoticed. A workflow runningtypecheck,lintandtestwould have caught every bug in this PR — but it cannot go green until #1 and #2 are fixed, so it needs to land after them.Happy to take any of these four as its own PR.