Seed the default apps once per user, not when the grid looks empty - #2149
Open
rbuergi wants to merge 4 commits into
Open
Seed the default apps once per user, not when the grid looks empty#2149rbuergi wants to merge 4 commits into
rbuergi wants to merge 4 commits into
Conversation
The home seeded the platform defaults (Store, Doc, Threads) from the render path whenever the viewer's app grid came back EMPTY. Emptiness was standing in for "this user has not been set up yet", and that proxy was only ever valid while nothing else could write an app record. The Store now writes one at install time (MeshWeaver.Plugins#618, merging today). So a user who acquires a package BEFORE first opening their home arrives with a non-empty grid, the seeding never fires, and they lose the defaults permanently — including the Store tile, which is the one the seeding exists to guarantee: a user without it has no way to reach the Store at all. Nothing errors; they just get a poorer home than everyone else, forever. It is a run-once logon action now, which says what the proxy was reaching for: once per user because the LEDGER says it has run, not because their grid happened to look a particular way on one render. That also fixes the same defect in the other direction, which nobody had reported: under the old trigger a user who deleted every tile had them silently restored on the next render. Run-once means removed stays removed. Two properties, both deliberate: - Create-if-absent, never overwrite. Each record is created independently and "already exists" is benign, so this cannot clobber a record the Store wrote for the same app, and a logon racing an install cannot duplicate one. - Idempotent on its own, because the framework's ledger is atomic with the user's PROFILE and not with these node writes. The action is therefore at-least-once and has to survive re-running. WHICH apps get seeded still comes from Admin/HomeConfig, so it stays deployment-specific; only "a new user must end up with the declared set" is framework behaviour. Removed with it: ObserveAppRecords, which existed ONLY to evaluate the emptiness proxy. The home now issues one fewer query per render, and the render path performs no app writes at all — with no exception, which the class doc now says. Full MeshWeaver.Graph.Test: 1577/1577. Release -warnaserror clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Moves default app seeding (Store/Doc/Threads) off the home render path and into a run-once logon action, so users always receive the deployment-declared starting tiles even if their app grid is non-empty on first visit.
Changes:
- Removed the empty-grid bootstrap write path from
UserActivityLayoutAreasand deleted the supporting query helper. - Added
SeedDefaultAppsLogonActionand registered it as a built-in logon action. - Added tests and a “What’s New” entry documenting the behavioral change.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.Graph.Test/SeedDefaultAppsLogonActionTest.cs | Adds unit tests pinning the logon-action trigger semantics and config-driven seeded set. |
| src/MeshWeaver.Graph/UserActivityLayoutAreas.cs | Removes render-time default seeding and the app-record emptiness probe. |
| src/MeshWeaver.Graph/Logon/SeedDefaultAppsLogonAction.cs | Introduces the run-once logon action that seeds default app records. |
| src/MeshWeaver.Graph/Configuration/LogonActionNodeType.cs | Registers the new built-in logon action in DI. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-24-seed-defaults-at-logon.md | Documents the user-visible behavior change. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Both correct, both mine. 1. My Skip(1) reintroduced exactly the failure this PR fixes. Observe() is StartWith(Defaults) + DistinctUntilChanged, so a portal with NO materialized Admin/HomeConfig node — or one whose config equals the shipped defaults — emits exactly ONCE. Skipping that emission to "avoid acting on the placeholder" waits forever on precisely the fresh deployment this action exists to serve: it times out and seeds nothing, including the Store tile. I removed one way to lose the defaults and added another. Now: take up to two emissions, stop at a 2 s settle bound either way, use whichever arrived last. One emission ⇒ the shipped defaults once the bound elapses; two ⇒ the configured set as soon as the query answers. Always terminates, and still never seeds the shipped set at a portal that declared its own. 2. The create helper swallowed EVERY failure. Combined with RunOnce that is the worse of the two: the runner commits the ledger entry on completion, so a transient fault — access denied, a missing parent, a storage blip — marked the action permanently done for that user while leaving them short a tile, with nothing that ever revisits it. Only "already exists" is tolerated now (that one IS success: it means the Store or a concurrent logon got there first); everything else propagates, the ledger stays unwritten, and the next logon retries. Free, because every step is idempotent. Tests pin both halves of the emission behaviour — the single-emission case that would have hung, and the two-emission case where the configured set must win. 62 home/seeding tests green; -c Release -warnaserror clean including the test project. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Maintainer feedback via the Plugins session, with the correlation already done: every small tile carried width/height on the icon's root tag, every filling one was viewBox-only. That reads as an authoring problem. It is not — or not only. A node icon is injected as a raw MarkupString, so Blazor's CSS isolation never stamps its scope attribute onto that <svg>. `.mesh-search-icon-box svg` was unscoped, so it matched NOTHING, and an authored width="24" rendered at 24px inside a 64px tile with no rule to override it. The row icon was never affected because its rule always had ::deep — which is exactly why the symptom correlated with authored dimensions and looked like the icons' fault. Two independent guards now, because either alone would have hidden this: - ::deep on the icon-box rule. The real fix: the stylesheet reaches the markup. - SizeInlineSvg on both icon paths, forcing the size onto the element itself. Duplicate attributes resolve first-wins, so the injected style beats both the authored attributes and any authored inline style. This is what stops the next authored icon with a fixed size from silently regressing the grid. Threads gets real artwork while I am here — a full-bleed gradient rather than a grey glyph that read as unfinished beside the Store's tile. Built to the three constraints that matter and are easy to miss: no root width/height (the bug above), attribute styling only (React Native renders neither <style> nor class-driven fills, so a class-based icon is invisible on the phone and fine on the web), and a namespaced gradient id (several inline SVGs share one document, and a duplicate id means the first definition wins for everybody). AppIconRenderingTest pins those three rules plus the first-wins ordering, so the next icon has something to fail against. HomeTabsTest now asserts the Threads icon by SHAPE, so the artwork can be redrawn without editing a test. Full MeshWeaver.Graph.Test: 1584/1584. -c Release -warnaserror clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Changing the Threads icon in the seed only reaches users who have not been seeded yet. Everyone already seeded keeps the grey chat.svg forever, because nothing revisits a record once it exists — so the redraw shipped to new accounts and silently skipped every existing one. That is a half-finished change and this is its other half; the Plugins session hit the same edge from the Store side and flagged that the default records are structurally out of ITS reach. 🚨 The rule is deliberately narrow: converge a record only when it wears an icon CORE ITSELF shipped and has since retired (AppIconAdoption.SupersededDefaultIcons, an explicit historical list). NOT "anything that differs from the current seed", which was the obvious formulation and is wrong twice over — it would overwrite an icon a VIEWER chose, which is theirs, and it would fight the Store, which converges the icons of records IT owns (MeshWeaver.Plugins#624). Two writers on one field with overlapping conditions is how a tile starts flickering between two answers on alternate logons. The list is enumerable precisely because it is HISTORY: we know what we shipped. Add a line when a default's artwork changes; a value that never shipped does not belong there. EveryLogon, not run-once, for the reason the action exists: a run-once refresh would converge whatever was stale on the day it ran, record itself as done, and strand every LATER redraw — the same bug one level up. Steady state is one query matching nothing. Disjoint from the icon adoption by construction: that one fills a record with NO icon, this one moves a record OFF a retired value, and the tests assert neither matches the other's case so they cannot race on the field. The write re-checks the condition inside the update, so a Store or viewer choice landing between the read and the write wins. Full MeshWeaver.Graph.Test: 1590/1590. -c Release -warnaserror clean. Co-Authored-By: Claude Opus 5 (1M context) <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
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.
The home seeded the platform defaults (Store, Doc, Threads) from the render path, whenever the viewer's app grid came back EMPTY. Emptiness was standing in for "this user has not been set up yet" — a proxy that was only ever valid while nothing else could write an app record.
Why it breaks now
The Store starts writing an
InstalledApprecord at install time in MeshWeaver.Plugins#618, merging today. So:Including the Store tile itself, which is the one the seeding exists to guarantee: a user without it has no way to reach the Store at all. Nothing errors and nothing logs — they simply get a poorer home than everyone else, forever. It is only reachable for users created after #618 lands, which is why this wants to be close behind it rather than after a gap.
The fix
Seeding is a run-once logon action (
SeedDefaultAppsLogonAction), on the framework that landed in #2142. That says what the proxy was reaching for: once per user because the ledger says it has run, not because their grid looked a particular way on one render.It also fixes the same defect in the other direction, which nobody had reported: under the old trigger, a user who deleted every tile had them silently restored on the next render. Run-once means removed stays removed.
Two properties, both deliberate and both load-bearing next to the Store's writer:
already existsis a benign no-op — so this cannot clobber a record the Store wrote for the same app (with its stamped name/icon), and a logon racing an install cannot produce a duplicate.Order = -100, ahead of the pin migration and icon adoption, since both operate on records. Not required for correctness — each is independently idempotent — it just avoids a first logon that heals nothing and then seeds.Which apps get seeded still comes from
Admin/HomeConfig, so it stays deployment-specific (memex and systemorph.com do not ship the same apps). Only "a new user must end up with the declared set" is framework behaviour. A portal that declares an empty set seeds nothing rather than falling back to the shipped list.Removed with it
ObserveAppRecords— it existed only to evaluate the emptiness proxy. The home now issues one fewer query per render, and the render path performs no app writes at all, with no exception. The class doc used to carve out that one exception; it no longer needs to.Tests
SeedDefaultAppsLogonActionTestpins the trigger rather than the payload, because the trigger is what was wrong: run-once mode, a stable ledger key (a rename re-runs it for every existing user, which for this action means re-creating records people deliberately deleted), ordering ahead of the record-consuming actions, the set coming from config, an empty declaration seeding nothing, and every seeded record landing inside the user's own RLS-scoped namespace.Full
MeshWeaver.Graph.Test: 1577/1577.-c Release -warnaserrorclean on Graph and its test project.Found while reviewing the Plugins-side install work with that session — it is their change that makes this reachable, and neither half is wrong on its own.