feat(seam): typed read per screen, 5 to 35 methods, all mock - #5
feat(seam): typed read per screen, 5 to 35 methods, all mock#5laksamanakeris wants to merge 26 commits into
Conversation
Result sets whose columns do not match the decoder's expectation now raise UnexpectedColumns instead of decoding to an empty list. The server answers some introspection statements with a session-variable fallback carrying a single 'setting' column, which would otherwise render as a working-but-empty screen.
The Row::field() method now reports short rows with a clearer expected value message, distinguishing row-too-short from column-not-found errors. Added short_row_is_an_error test to cover the previously untested branch where a row has fewer cells than the table has columns.
The client defaults an unset trust username to admin and the server no longer supplies its own default, so a blank field would connect as admin without telling the user. Blank is now a typed MissingUsername error.
MockBehavior moves out of connection_service and gains Delayed, so every domain method added later reaches all four async states through one apply helper instead of re-implementing the match.
Sidebar groups, list rows and record detail now have typed seam methods. Grouping happens behind the seam because storage mode is not available in a single server call, so the sidebar never sees that resolution.
record_detail had no test at all: add ready/erroring/empty cases and pin down that Empty folds into the same success path as Ready for a single-value read. groups_are_ordered_and_non_empty only checked non-emptiness; now it asserts the group sequence matches StorageMode's canonical order. Also standardize every SEAM-UNWIRED allow(dead_code) in these files onto one greppable tag instead of three different explanatory comments.
Cluster, raft, shards, RBAC, RLS and audit reads now flow through the seam with rich mock data and a NotConnected stub, so no admin screen is a special case.
Add empty/erroring coverage for shard_ranges, users, rls_policies and audit_entries so a method mis-wired as Ok(mock::x()) instead of apply(self.behavior, mock::x) would fail. Assert the admin/alice is_superuser flags and the RLS policies' enabled flags, and give rls_policies a second, distinct entry so its uniqueness check is real instead of vacuous. Reword the admin models doc comment: no bool parsing happens yet, it describes the seam's future decode boundary.
…tify Stream reads are idempotent and only an explicit commit advances the cursor, so the seam now models open/read/commit/close with a Studio-owned consumer group. Sharing a group and committing would advance a production consumer past unprocessed events.
cdc_batch recorded cdc_read_end from the computed batch before consulting self.behavior, so an empty or erroring read still moved the cursor as if every row had been delivered. A following commit then promoted that phantom offset into cdc_committed, silently skipping events the caller never saw. Now cdc_read_end is set only from what apply actually delivered. Also gives MV/Topic/ScheduledJob/NotifyChannel fixtures distinct id/name pairs so a wrong-field-keyed list bug is visible.
Result sets are paginated at the seam because the client buffers whole result sets with no cursor.
… and sync Models carry their own display fields because the client decodes graph properties and vector metadata as empty; the real implementation populates them from raw rows.
Shell chrome reads through the seam instead of rendering literals.
…ipline gate Split ExplorerSidebar into a fetch wrapper (use_resource over Backend::collection_groups) and a presentational SidebarGroups component driven by AsyncState, following the CDC screen's pattern. Groups and collections are keyed by StorageMode::key()/name instead of Debug/index. Removed the now-orphaned mock::explorer_collections fixture (superseded by mock::collection_groups) and the SEAM-UNWIRED(task-10) tags on the collection_groups path now that it has a real non-test caller. Added tests/seam_discipline.rs: an integration test that scans src/views, src/components and src/modals for data::mock/mock:: references outside code comments, failing with file:line detail on any new violation. views/streams/notify.rs is the one documented exception (its seam models are missing fields the current view renders; rewiring is deliberately deferred), verified to actually fail without the exception before restoring it.
Six seam methods (session_info, nav_badges, record_detail, run_query, explain, sub_graph) returned a single value rather than a Vec, so they could not satisfy AsyncState<T>'s IsEmpty bound and were unrenderable through the AsyncView primitive. Add explicit IsEmpty impls (always false, a fetched single value is never "empty") for each of the six models, with a test proving from_value(Some(Ok(_))) now yields Loaded. Also collapse the six byte-identical MockBehavior match blocks in connection_service.rs into a new apply_one helper (mirroring apply's four-arm semantics), and route list_connections/notifications/connect through apply/apply_one so all nine mock methods honour every MockBehavior variant, not just the six added most recently. connect's blank-username guard still runs before any behaviour branch.
decode.rs and error.rs used bare #[allow(dead_code)] instead of the project's #[allow(dead_code)] // SEAM-UNWIRED(task-10) convention, hiding the decoder module from the grep that enumerates unwired seam surface.
- models/streams.rs: delete stream_session_carries_stream_and_studio_group, a tautology that only echoes the struct literal it builds. - viewers_data.rs: every_viewer_read_is_keyed_and_non_empty now actually asserts every item's id is non-empty, matching what its name claims; previously it only checked collection non-emptiness. - viewers_data.rs: delete empty_and_error_states_reachable, fully subsumed by the later per-method vector_points_empty_is_empty and sync_peers_erroring_is_err.
Credentials derived Debug on a struct carrying an Option<String>
password. Harmless while the field is always None, but the moment the
connect form populates it, any {:?} or error-chain print would leak
the secret. Hand-write Debug instead: it prints username verbatim and
always renders password as a fixed "<redacted>" marker, whether Some
or None, so a print cannot even reveal presence-vs-absence.
mock::nav_badges() returned streams: 2 while components::rail still renders a hardcoded Streams badge of "6". Update the fixture to 6 so the eventual swap onto the seam is a visual no-op; the rail itself is left unwired, as that is a later phase.
The Explorer hardcoded its default selection as "events" / Document, a collection that does not exist in collection_groups() (an earlier fixture change dropped it). Opening the Explorer showed a viewer header for a collection the sidebar didn't have, with no row highlighted. Replace the hardcoded literal with default_selection(), which derives the default from the loaded data (first collection of the first group). Selected is now Option<Selected>: there is no selection at all while the seam read is loading, empty, or errored, and the main pane says so instead of fabricating a name.
This repo is public; the plan that defines task/phase numbers is not, and the project's rules ban roadmap markers in source, comments, and test names. Strip the "(task-10)" suffix from every SEAM-UNWIRED marker, and reword the seam_discipline notify exception to state the substance (the seam models are missing fields the notify view renders) instead of citing a numbered review. SEAM-UNWIRED itself is kept: it says something a reader can act on (the seam is ahead of the views), and `grep -r SEAM-UNWIRED` already enumerates every site without the numeric suffix.
All three connect call sites (Connection Manager, the connection switch popover, the command palette) built the username with .unwrap_or_default() and then discarded connect()'s Err with `if let Ok(..)`. A connectable saved connection with a missing profile would default its username to blank, get rejected by the seam's MissingUsername guard, and have that error swallowed silently: a Connect button that does nothing, with no error and no state change. Match on the result at each call site and log the failure via tracing::error!. Also add a fixture invariant test asserting every connectable entry in mock::connections() has a profile, so the coincidence that hides this today breaks loudly the moment a fixture changes. While touching these call sites, drop the "(later phase)" parenthetical from their TODO comments — the sentences read fine without a reference to an unpublished plan.
data/mock/streams.rs establishes the convention that a fixture's id must differ from its display name, with an assert_ids_distinct_from_names helper precisely so a list keyed by the wrong field is visible instead of invisible. data/mock/admin.rs's users() and rls_policies() violated that same convention (id == username / id == name), on this same branch. Give both fixtures distinct id/name pairs, move the shared invariant helpers into a new test_support module so streams and admin tests can both reuse them, and update admin_data's fixture-content tests to look users and policies up by username/name instead of id now that the two differ.
ResultSet and SubGraph's IsEmpty impls returned a hardcoded false, on the reasoning that "a fetched single value is never empty" — true for SessionInfo/NavBadges/RecordDetail/QueryPlan, false for these two: a zero-row result set and a zero-node graph are the empty case, and "no rows" is the most common non-error workbench outcome. The Query workbench and graph viewer could never reach AsyncState::Empty, so a zero-row query rendered as a blank pane indistinguishable from a broken one. Make both impls honest (rows.is_empty() / nodes.is_empty()), and give run_query/sub_graph a way to actually deliver that empty payload: add apply_one_or_empty to mock_behavior (like apply_one, but Empty calls a caller-supplied empty thunk instead of folding into Ready), and add empty_result_set/empty_sub_graph fixtures. The four single-valued models keep folding Empty into Ready via apply_one, unchanged. Split the async_state test that pinned ResultSet/SubGraph into the buggy `false` behaviour: the four genuinely single-valued models keep their Loaded-not-Empty assertion, while ResultSet/SubGraph get the opposite (empty payload -> Empty, non-empty -> Loaded). Also parameterize ViewersData's sub_graph, vector_points, spatial_features and fts_hits with a collection (fts_hits also gains it alongside its existing query param), matching the pattern records() already uses — the Explorer already scopes graph/vector/spatial collections by selection, so wiring the per-mode viewers later would otherwise be a signature change across four methods and both implementors. sync_peers stays instance-scoped. Mock fixtures now key every id off the requested collection so a wrong-argument bug is visible, with tests asserting the variation. Point the shell.rs and viewers_data.rs module docs at apply_one / apply_one_or_empty instead of a nonexistent "explicit four-arm match" — every seam method here already went through apply_one.
- connection_service.rs line 577: remove `run_query` from citation since it no longer folds Empty into Ready; keep `record_detail` reference. - workbench.rs lines 32-33: correct claim that columns make empty state distinguishable; they are shape documentation only, and proper rendering would require a payload-carrying Empty variant that does not exist.
Second review pass, six fixes appliedRan an independent review against the standards the server repo enforces. It returned BLOCK. Six commits pushed since; the re-review now returns APPROVE. Two blocking issuesThe Explorer showed a collection that did not exist. The default selection was the hardcoded literal It survived every prior check because the sidebar's render tests build their own fixture and their own selection signal, so the real fixture was never rendered against the real default. Fixed structurally rather than by changing the literal: selection is now Task identifiers referencing a document readers cannot open. 85 Four design issues
A test was pinning that defect as the contract, so a later fix would have hit a failing test whose name claimed the behaviour was intentional. Split, with the two affected models now asserting the opposite.
Connect failures were silent. All three call sites defaulted a missing username to blank, had the seam reject it, then discarded the error. A profile-less connectable entry would have produced a Connect button that did nothing at all. Errors are now handled at all three sites, and a test asserts every connectable fixture entry has a profile so the invariant cannot drift. Plus two fixture and comment corrections. VerificationUp from 194 when this PR was opened. Still deferred, unchangedThe notify view still reads fixtures directly behind a named exception in the seam-discipline test, and the remaining One thing the review raised that is worth flagging rather than fixing here: the seam is read-only, and the two existing writes have no read invalidation. Every screen phase after this adds mutations, and each wired view currently owns its resource handle locally, so a mutation in one component cannot refresh another. Worth settling with one write wired end to end before the screen phases start, rather than discovering it five screens in. |
laksamanakeris
changed the title
1 participant
Grows the backend seam from 5 methods to 35, one typed read per data-bearing screen, so every screen after this is "fill the seam" instead of "reshape the seam." Still 100% mock data. No real client is called anywhere.
Read this first (5 minutes)
-D warnings, nextest, all cleanThe one design choice to agree or disagree with: seam methods are decoder-shaped. They return Studio model types buildable from
QueryResult{columns, rows}, never a client type. Reason: the typed client covers 3 of 9 engines; everything else comes back fromSHOW …SQL as column-addressed string rows. If that premise holds, screens wire up by writing decoders. If you think it is wrong, say so here, because five screen tiers build on it.Where to spend review time
In order of consequence:
services/decode.rs— asserts expected column sets. The server answers someSHOWstatements with a session-variable fallback carrying a singlesettingcolumn. A decoder that just looked for its columns would find none and render an empty list, so the screen would read "working but empty." This turns that into a typedUnexpectedColumnserror. Small file, load-bearing.services/streams_data.rs, the CDC lifecycle — open / read / commit / close with a Studio-owned consumer group. Reads are idempotent and onlyCOMMIT OFFSETSadvances the cursor. The mock reproduces that: a read that returned nothing or failed leaves the cursor untouched, so a later commit cannot skip events nobody saw. Worth checking the mock matches your understanding of the server.connectnow requires an explicit username. The client defaulted an unset trust username toadmin; the server dropped its own default. A blank field would have connected as admin silently. Now a typedMissingUsername, checked before any other branch.Credentialshas a hand-writtenDebugthat redacts the password identically whether set or not.services/viewers_data.rs— the graph, vector and spatial reads take acollectionparameter. Earlier draft did not; the Explorer already swaps viewer panes per collection, so wiring them later would have been a signature change.Safe to skim
data/mock/<domain>.rsfixture files. Deterministic, keyed, vary by input so a wrong-argument bug is visible.NodeDbConnectionServicestubs. Every method returnsNotConnected. They exist so both implementors compile as aBackendand neither is special-cased.tests/seam_discipline.rs— fails the build if any view, component or modal reads fixtures directly.Deferred on purpose, and contained
activeandsource, which that view renders, so rewiring needs a UI decision. The gate test names it as the one exception and fails if it ever stops needing one.#[allow(dead_code)] // SEAM-UNWIREDmarkers. One view is wired; the rest of the seam has no caller yet. Uniform, greppable, cleared as each screen lands.Review history
Two independent review passes before this reached you. The second, held to the server repo's standard, returned BLOCK on a phantom-collection regression (the Explorer's hardcoded default named a collection the new fixture did not contain) and on task identifiers in source. Both fixed, plus four design items. Details in the comment below. Re-review returned APPROVE.
Verification