feat(seam): typed read per data-bearing screen, 5 to 35 methods, all mock - #5
Open
laksamanakeris wants to merge 36 commits into
Open
laksamanakeris wants to merge 36 commits into
laksamanakeris wants to merge 36 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.
The popover cleared every badge in the local store before sending the write to the seam. If the write failed, the error went to a log line and the screen stayed cleared while the server still held the items unread; on the next reload the badges reappeared with no explanation. A first attempt at this fix folded the write failure into the shared read store as an error. That reproduced the original symptom by another route: the header derives its unread count from the loaded list, which an Error state does not have, so it rendered "all clear" beside an error box, the bell badge dropped to zero, and the Retry button re-ran the read rather than the write. The write failure now lives in its own signal. The loaded list stays on screen, because the read did not fail and the user was looking at correct data. The failure renders beside the list through AsyncView, the same component every failed read uses, so it is styled, tested and gated on retriability in one place; its Retry re-issues the write. The reconcile step is a plain function in state/notifications.rs so both outcomes are unit-tested without a renderer: Ok clears the list, Err leaves it exactly as it was and hands back the error. The Err test fails against optimistic clearing. The mock write ignored MockBehavior, so its failure path was unreachable in tests; it now routes through the same behaviour switch as every read. The shared-state test helper is renamed from with_shared_cursor to with_shared_state, since it shares the read-flag cell too and one test was rebuilding that by hand. The streams_data docstring that described mark_all_read as ungated now states the contrast instead.
laksamanakeris
changed the title
All three connect entry points (the Connection Manager card, the command palette, the switch popover) matched on the seam result and sent the Err arm to tracing. An earlier pass had already moved these from `if let Ok(..)` to `tracing::error!`, which reads as a fix but changes nothing the user can see: clicking Connect on an entry the seam rejects produces no message, no state change, and no clue that anything happened. The seam's own `MissingUsername` guard is reachable this way, so the most likely failure was also the most invisible one. The error surface is provided at the app root, not by the components that start the attempt: the palette and the popover both close themselves on click, so an error either of them owned would be dropped before it could render. `ConnectError` is a newtype rather than a bare `Signal<Option<StudioError>>` because Dioxus keys context by type, and a second bare-error provider added later would bind to this one silently. It renders through AsyncView, the same component every failed read uses, so markup and styling stay in one place. `retriable` is false by construction: the way to retry a connect is the button the user just pressed, which is still on screen, and a second affordance here would need the name and credentials of the attempt that failed. The message clears when the next attempt starts and when one succeeds. Reconciliation is a plain function in state/connection.rs, so both outcomes are unit-tested without a renderer. It also pins an invariant the old code only held by accident: on Err the existing session is left untouched, because a failed switch must not disconnect the user from the connection they still have. That test fails against an implementation that clears `active` on Err. A structural test counts connect() call sites against apply_connect() reconciles under views/components/modals. This regression already shipped once looking fixed; a fourth call site that forgets to surface its failure now breaks the build instead of the screen.
The form was a static mockup: uncontrolled inputs with hardcoded values, and a "Save & connect" that only closed the modal. Phase 1 asks for the opposite of a silent default here — the seam already refuses a blank username with MissingUsername rather than letting ConnectionBuilder default it to `admin`, and the form is where a user is supposed to be able to supply one. Name, Username and Password are now signals, and "Save & connect" runs the same reconcile path as every other connect site. Submit is blocked on a blank or whitespace-only username, with the message under the field; the message is raised by the submit attempt, not by typing, so the form does not scold a user who has not filled it in yet. On failure the modal stays open over the rendered error so the field that caused it can be corrected; it closes only on success. The validation is a plain function so it is tested without a renderer, matching how the notification and connect reconciles are structured. Its tests pin two things the inline version got to decide implicitly: whitespace is not a username, and an empty password field is an absent password rather than a password of length zero. Host, port and auth method stay presentational, and so do Test and Save: connect() takes a name and credentials only, and no seam method persists a new entry. Wiring those needs a seam addition, which is ask-first. Also retires three stale TODOs claiming the saved-profile username is a placeholder for a field that does not exist yet. For a saved connection the stored profile IS the explicit username; the case those comments worried about (a profile-less entry) produces a blank that the seam rejects and the app root now renders, so the behaviour is stated instead of deferred.
The notify view read data::mock directly, the only screen still doing so, carried as a documented exception in the seam-discipline test. The stated reason was that the seam models lacked the `active` and `source` fields the view rendered. Both halves of that turned out to be wrong. `active` marks which channel the user clicked. That is view state, and the server has no opinion about it, so it was never a missing model field. It is now derived the way the Explorer derives its default selection: the first channel the seam returned, with no hardcoded name to rot when the fixture changes. `source` was a publisher identity per message. NodeDB has no LISTEN/NOTIFY at all — no LISTEN keyword in nodedb-sql, no pub/sub, no client path; the only NOTIFY in the server is CREATE ALERT ... NOTIFY TOPIC/WEBHOOK, which is alert routing. Nothing can ever populate that field, so the tail shows the channel rather than carrying a seam field no decoder could fill. The module comment records this, because the next person to read the screen will otherwise assume a backend exists. Structure follows the CDC view: a fetch wrapper owning the two reads and a pure presentational component taking AsyncState, so all four states are render-testable. The tail is scoped to the selected channel, which is what makes picking one mean anything, and the toolbar's listener count is read off the loaded channel instead of a literal so it cannot disagree with the sidebar. ALLOWED_EXCEPTIONS is now empty and the test guarding that one exception against going stale is gone with it. The pre-seam fixture file is deleted rather than left orphaned, and the SEAM-UNWIRED allows on the models, fixtures and seam methods this wires up are removed (87 markers to 81).
decode.rs claimed every value arrives as a string. That is true for the SHOW/DESCRIBE surface this module decodes, and false for native SELECT: NodeDB [Unreleased] returns nested objects and arrays as structured values rather than JSON text. Those shapes do not fit Table's Vec<Vec<String>> rows and belong in models::workbench::ResultSet. The comment now says which path it covers, so nobody builds a SELECT decoder on a premise that only holds for the catalog. Seven comments cited CLAUDE.md by section number. CLAUDE.md is a one-line import of AGENTS.md and has no numbered sections, so every one of those pointers led nowhere. They now state the constraint inline, which also keeps the public repo from referring to documents outside it.
`spawn` binds a task to the calling scope, and Dioxus drops a scope's tasks when the scope is removed. The switch popover is conditionally mounted and closes itself on the line after it spawns, so the connect future was dropped at its first await point: no session, no error, no trace. Clicking a connection in the popover would do nothing at all, which is the exact failure the previous commit set out to remove. The mock hid it. `apply_one` resolves on the first poll, so the task finished before the unmount could cancel it. Any backend that actually yields — a real client, or MockConnectionService::delayed — reaches the await and loses the task. Every connect site now uses spawn_forever, which runs on the root scope. The popover is the one that was provably broken, but a connect has to outlive whichever component started it in all four cases: the palette closes on click, the connection manager is swapped for the studio shell on success, and the modal can be cancelled mid-flight. Also fixes the command palette reading the registry with `peek()` at render time. peek() does not subscribe, so the credentials froze at the render where the palette opened; against a backend whose connection list resolves later, every switch would send a blank username and the palette would never re-render to correct itself.
The connect-error banner rendered as a normal-flow element at the app root, which broke it in two ways. It was invisible exactly when it mattered most. `.modal-overlay` is fixed at z-index 150 with a 40% black scrim, so a failed "Save & connect" put the error behind the scrim and above the viewport, under the form that caused it. The modal deliberately stays open on failure so the user can correct the field, which only helps if the reason is readable. It also clipped the shell. `html, body` are 100vh with overflow hidden, and both `.app` and `.conn-manager` are themselves 100vh, so a banner in front of them pushed the bottom of the page out of view with no scrollbar. In the connected state that hid the statusbar. It is now fixed-position at z-index 200, above the scrim, taking no space in the flow.
The tail is scoped to the selected channel, but the empty flag came from the unfiltered seam read. Every fixture message is on `user_events`, so selecting `deploy_hooks` or `cache_invalidate` produced zero rows and no "No messages." — a blank pane, which is the thing this screen's own module comment claims the design avoids. The flag now comes from the filtered rows, with Loading and Error taking precedence so neither the spinner nor the error text is replaced by an empty message. A first attempt keyed it off `loaded().is_some()`, which fails for a genuinely empty read: that maps to AsyncState::Empty, where `loaded()` is None. The test caught it. Also stops the header claiming a count before the list arrives. It read "Channels (0)" directly above its own spinner or error.
The effect kept any existing selection so a reload could not clobber a user's pick. It never checked the pick still existed. A reload returning a set without that collection left the viewer header naming it while no sidebar row matched — the same phantom-collection symptom the hardcoded `events` default used to produce, arriving by a different route. The selection now survives only while the collection does. The check is a plain function beside `default_selection`, so it is tested without a renderer. Also makes the connect-reconcile gate compare per file rather than as a global total. A single tally let a file that dropped its result pass because another file contributed a spare `apply_connect` line.
Same defect as the connect sites, found by looking for the pattern rather than the symptom. The notification popover is conditionally mounted, and its mark-all-read handler used a scope-bound spawn. Clicking away while the write was in flight killed the task at the await: the server's state becomes unknown and the shared store is never reconciled. spawn_forever runs it on the root scope. The store is app-level context so the reconcile still lands. `write_error` stays scoped to the popover, so a failure the user navigated away from is not displayed; that is acceptable because the failure path leaves the list untouched, which is exactly what reopening the popover shows. Adds a source rule banning scope-bound spawn under views, components and modals. Seam writes use spawn_forever, seam reads use use_resource or use_future, which are tied to the component deliberately. The rule is a source scan rather than a runtime test because the mock cannot reproduce the bug: apply_one resolves on the first poll, so the task always finishes before an unmount could cancel it. Every test in this repo passed against all eight broken call sites.
laksamanakeris
changed the title
laksamanakeris
changed the title
laksamanakeris
changed the title
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.
Grows the
ConnectionServiceseam from 5 methods to 35, one typed read per data-bearing screen, backed by a typedmodels/layer and per-domain fixtures the mock can return as empty, error or delayed. Still 100% mock. No real client is called anywhere.-D warnings, nextest, all cleanThe design choice to accept or reject
Seam signatures are decoder-shaped. Every method returns a
models/struct buildable fromQueryResult{columns, rows}, never a client type.The typed client covers 3 of 9 engines. Everything else arrives from
SHOW …SQL as column-addressed string rows, so the unit of work per screen is a typed struct plus a decoder. If that premise holds, later phases are "write the decoder". If you think it is wrong, say so here, because five screen tiers build on it.Where to spend review time
1.
services/decode.rs. Asserts the expected column set before decoding. SomeSHOWstatements fall through to the session-variable handler and returncols=["setting"]with one empty row instead of erroring. A decoder that only looks for its own columns renders an empty list, so the screen reads as working but empty. This turns that into a typedUnexpectedColumns.2.
services/streams_data.rs, the CDC lifecycle. Open / read / commit / close against a Studio-owned consumer group. Reads are idempotent and onlyCOMMIT OFFSETSadvances the cursor, so a poll loop on a shared group would walk a production consumer past unprocessed events. Studio createsstudio_<stream>on connect and drops it on disconnect. The mock reproduces this: a read that returned nothing or failed leaves the cursor untouched. Worth confirming the mock matches the server.3.
connectrequires an explicit username.ConnectionBuilderdefaulted an unset trust username toadmin, and the server dropped its own default, so a blank field connected as admin silently. Now a typedMissingUsername, checked before any behaviour branch. The new-connection modal validates blank before it reaches the seam.Credentialshas a hand-writtenDebugthat redacts the password whether set or not.4. Connect failures reach the screen. All five call sites reconcile through
apply_connectand render throughAsyncView, the same component every failed read uses. Previously they matched on the result and sent theErrarm totracing, which is invisible to anyone looking at the app. A per-file gate intests/seam_discipline.rsfails the build if a call site stops reconciling.Two things I want your call on
1. Is the
spawn_foreverrule right?spawnbinds a task to the calling scope, andremove_scopedropsspawned_taskson unmount (dioxus-core-0.7.10/src/runtime.rs:187). Most of this tree is conditionally mounted: every popover, every modal, andConnectionManageritself, which is swapped forStudiothe moment a connect succeeds. Six handlers spawned a seam call and then closed their own scope, so the future died at its first await. Five connects andmark_all_read.The mock cannot catch this.
apply_oneresolves on the first poll, so the task always beat the unmount. All 235 tests passed against all six.Seam writes now use
spawn_forever; reads stay onuse_resource/use_futuredeliberately.tests/seam_discipline.rsenforces it as a source scan, since a runtime test cannot. That rule is the only thing holding this class until a real client lands, so it needs your eyes rather than CI's.2. Does Streams · NOTIFY belong in this milestone?
NodeDB has no LISTEN/NOTIFY. No
LISTENkeyword innodedb-sql, no pub/sub, no client path. The onlyNOTIFYisCREATE ALERT … NOTIFY TOPIC/WEBHOOK, which is alert routing. SoNotifyChannel,NotifyMessageand their two seam methods stand for a feature that does not exist.The view is seam-backed now, so
ALLOWED_EXCEPTIONSis empty. The mockup showed a per-message publisher identity; nothing can produce it, so the tail renderschannelrather than carrying a field no decoder could ever fill.Deferred on purpose
// SEAM-UNWIREDmarkers. Uniform and greppable, cleared as each screen lands.