feat: 2.0 shell, routes, and per-user shell selection - #1247
Merged
Conversation
Six tasks covering the route table, the mobile bottom nav, session bootstrap, the hub, the profile page, and the server-side flag that decides which shell a request is served. Four judgement calls are recorded in the plan rather than left implicit: the router is react-router since it is already a dependency, component tests run on bun's own runner instead of adding Vitest, the profile reads the existing settings endpoint so nothing here waits on the transport work, and the spec's list of things this IA deletes stays out of scope because those files still serve the shell most users are on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Wrap the route table in a layout route rendering Shell, which owns the bottom nav and an Outlet for the page content. ROUTES stays flat for Task 1's test; the nested table createBrowserRouter consumes is exported separately as ROUTER_ROUTES. happy-dom needs registering before any test file imports @testing-library/dom, which only bunfig's preload can guarantee, but that preload runs once for the entire bun test process across every workspace in this monorepo, not just app/. Registering happy-dom there unconditionally broke packages/masterfile (CORS), server/test (Response.json resolving to happy-dom's class), and app/build.test.ts (Vite's bundled code branching on typeof document). test-setup.ts instead exports setupDom/teardownDom for a test file's own beforeAll/afterAll, which Bun scopes per file, and BottomNav.test.tsx reads its queries off render()'s return value rather than the testing-library screen singleton, since screen snapshots document at import time and would need the same process-wide registration.
app/test-setup.ts deliberately does not register happy-dom at import time, because a preload applies to the whole `bun test` run and there is no per-directory scoping, which broke three unrelated suites when tried. The file only exports setup and teardown functions now, and the one test that needs a DOM imports them and calls them from its own beforeAll. That leaves the preload entry doing nothing. Removing it keeps the suite at 80 passing, and it stops the config implying a global registration that is not happening, which would mislead anyone who later added a side effect to that file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds a boolean useAppShell column to the users table, defaulting to false so every existing row keeps the 1.0 client, and teaches the client router to pick between dist/app.html and dist/index.html per request. The route list is now the union of both clients' paths, so 1.0 deep links keep working and the flag, not the path, decides which shell a visitor receives.
The pre-2.0 migrations resolve this table through database.settings.userTableName so an install could rename it. 2.0 drops that, and the change was announced as unsupported long enough ago that designing the new migration around it would be carrying weight nobody is standing on. An install that did rename the table now fails at migrate time with a missing-table error rather than quietly upgrading half way, which is the better of the two failures. The existing migrations keep reading the config value. They describe work already applied, and rewriting them would change the account of what ran. Removing the option from the config and the models is its own job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Discord and Telegram spread the whole user row onto the session user, so they picked up the new useAppShell column for free. Local login assigns fields one at a time and did not copy it, so flagging a local account onto the 2.0 shell silently did nothing and the account stayed on 1.0. That is the failure mode worth avoiding here: nothing errors, the column holds the value asked for, and only the one auth method quietly ignores it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…path Guards against the local-login gap fixed in f52518b, where Discord and Telegram spread the whole row and picked up the new column for free while LocalClient copied fields one at a time and silently dropped it. Verified against the pre-fix LocalClient to confirm the local-login test actually fails without the copy line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Guards the gap that let a local account be flagged onto the 2.0 shell and silently stay on 1.0. Discord and Telegram spread the users-table row onto the session user, so a new column reaches them for free; local login copies fields one at a time and had to be told about this one. Driving the login end to end would mean standing up app config that only exists after a real server boot, which is a lot of scaffolding for one field assignment, so the invariant is pinned against the source instead. It is written as "spreads the row or copies the flag", not "keeps using a spread". Rewriting a client to assign fields explicitly is fine as long as the flag comes along, and a test that failed on correct code would get deleted the first time it was in the way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The handler was registered over the union of both route tables while the shell decision looked only at the user, so a flagged account asking for one of the 13 paths the 2.0 client has no route for got app.html and fell through to its catch-all NotFound. Shared map deep links, the blocked page and password reset were all among them. Register the 1.0 only paths, derived from the two tables so a third list cannot drift, with a handler that always sends the 1.0 shell, and let the flag decide only on paths both clients implement. The existing tests checked list membership, which is why this survived review. The new ones drive the router over HTTP and assert the shell each path returns under both flag states.
serve-static answers a directory request with index.html by default, and it is mounted ahead of the router, so a cold load of / was served the 1.0 shell off disk and never reached the code that picks a shell per user. Navigating to / inside the 2.0 client worked, since that is client side routing, which is why this went unnoticed. Passing index: false hands / to the router that already owns it. Hashed assets still come off disk, which the new test checks alongside the fall through.
Passport serializes the whole users row into the session and deserializes it without re-reading the database, so flipping the column did nothing until the person logged out. Sessions live in the database, so a restart did not help either, and someone hurt by a bad flag could not be rescued by setting it back. The settings endpoint already re-reads the row, so the refresh happens there. It patches the object stored in the session as well as req.user, because the deserializer hands back a copy and a req.user patch is discarded when the request ends. The flag then applies on the next page load, since the shell is chosen when the HTML is served and the settings call happens after it. The new test drives the real serializer pair over an express session, and fails if only req.user is patched.
CI failed on "marks the active destination for assistive tech" with TestingLibraryElementError: Found multiple elements with the role "link" and name "Filters". It passed locally every time. Renders are appended to the same document and stay there, and the queries render returns are bound to the whole body rather than to the container that render created. The hub renders a link labelled Filters as well, so once both files had rendered there were two matches and getByRole threw for ambiguity. Whether that happened came down to which files had already run, which is why one machine saw it and the other did not. Both files now clean up after each test, and queries are scoped with within() to the container the render owns, so a stale render elsewhere cannot make them ambiguous. Verified against a deliberately polluted document holding two competing Filters links: the scoped query still resolves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Plan 2 of 5. The 2.0 client gets a shell you can actually navigate: every route from the spec's information architecture, a mobile bottom nav, a hub at the root, a profile page, and a per-user flag on the server deciding which built shell a request is served. 1.0 is untouched apart from the routing change that has to know about both.
What is here
Eight routes in
app/, each its own lazy chunk. The map route is the only one that will ever carry MapLibre and deck.gl, and that only holds if nothing pulls a page in at module scope, soapp.htmlis checked to load no page chunk eagerly.A bottom nav with Map, Filters, Alerts and Me. Search stays in the map's top bar and the hub sits behind the logo, per the spec.
A hub at
/that renders without a session at all, because it is on for every operator including anonymous ones. It is navigation and nothing else. The news feed is a later phase.A profile page that bootstraps from
GET /api/settings, the endpoint 1.0 already uses. That was deliberate: it means none of this waits on the transport work, which is a separate design session.A
useAppShellcolumn on the users table, defaulting to off, and a router that reads it. Nothing writes it yet, so flagging an account is a manual database update today. That is the intended state for this plan./map,/filters,/alertsand/profilerender placeholders. Filling them is what plans 3 through 5 are for.What the review caught
Three findings on the routing, all of them design mistakes in the plan rather than the implementation, and all three worth describing because they share a shape.
The shell decision was made per user and applied to the union of both route tables, so the path never entered into it. A flagged user was served the 2.0 shell for the thirteen paths only 1.0 implements, including the
/@/lat/lon/zoomand/id/category/iddeep links people share, plus/resetand/blocked/:info. Every one of them fell through to the 2.0 catch-all and rendered a not-found page. Flag an account, open a map link a friend sent, get nothing. A path only 1.0 owns now serves the 1.0 shell whatever the flag says.The hub was unreachable by URL.
express.staticmounts ahead of the router and serve-static defaults to servingindex.htmlfor a bare directory request, so/was answered off disk before the router ever saw it. Navigating to/from inside the app worked, which is why nothing noticed: only a cold load or a bookmark showed it. Static now passesindex: falseand lets/fall through.Flipping the column did nothing until the user logged out, because passport serializes the whole user into the session and hands it back without re-reading. Worse in reverse, since a user hurt by the first finding could not be rescued by setting the flag back to zero. The fix re-reads the column on session init and writes it to both the request user and the stored session object, because patching only the former is discarded at the end of the request.
Worth naming how these were found. Three review passes ran. The two that read the diff and ran the build cleared the branch. The one that mounted the real router behind the real middleware and issued real HTTP requests found all three. The existing tests could not have caught any of it: they called the decision function and checked list membership, which verifies the decision and never the consequence.
Known limitations
An unflagged user hitting a 2.0-only path lands on 1.0's not-found page, the mirror image of the first finding above. Two shells with different route vocabularies cannot both be complete, and this only bites while accounts are being flagged by hand.
The flag takes effect on the next page load rather than instantly, since the shell is chosen when the HTML is served and the settings call happens after it.
res.sendFileinherits send'sdotfiles: 'ignore', so an install path containing a dot segment returns 404 for every client route while static assets keep working. This predates the branch and is left alone.Verification
The route table was exercised as a full matrix, every path against both flag states, plus a user with no session, an empty user, the column absent, false, and a mysql tinyint zero. All thirteen 1.0-only paths return the 1.0 shell in both states, the seven 2.0 paths honour the flag, and every degenerate user shape falls back to 1.0. Each of the three fixes was reverted in turn to confirm its test actually goes red.
🤖 Generated with Claude Code