fix(app-shell): overlay recovery strips a half-filled between (#5025) - #5657
Merged
Merged
Conversation
`sanitizeViewOverride` carried the shape-blind emptiness predicate that
objectstack#8815 retired on the two builder write paths. It is correct for
`scalar` and `list` and blind to `pair`: a `between` with one bound typed is
`['2024-01-01', '']`, an array of length 2, so the recovery pass read it as a
real condition and handed it back to the `{ ...source, ...override }` merge.
The server refuses that range outright (`400 INVALID_FILTER`), and because the
row is stored the refusal returned on every later read of the view, for every
user of it.
Both branches now delegate to the builder's arity-aware
`isFilterValueComplete` — the same helper `plugin-list`'s `ListView` and
app-shell's own `viewFilterFold` already read, imported from the same public
entry, so no export was widened to reach it.
Adds `ObjectView.overlayPairValue.test.ts`, whose `pair` cases are measured red
against a leg restoring the old expression while the `scalar` / `list` block
stays green.
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-sales
marked this pull request as ready for review
August 22, 2026 01:59
This was referenced Aug 22, 2026
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
2 participants
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.
Fixes #5025
sanitizeViewOverrideinpackages/app-shell/src/views/ObjectView.tsxcarried the shape-blind emptiness predicate that objectstack#8815 / PR #4962 retired on the two builder write paths:Correct for
scalarandlist, blind topair. Both branches now delegate to the builder's arity-awareisFilterValueComplete.Reachability of the reuse target — the card's first question
Verdict: reachable as a plain import, no widening. Nothing was exported to make this work.
The chain, verified on the built type surface rather than on source (an unbuilt or stale
distlies in both directions):packages/components/src/custom/filter-builder.tsx:840—export function isFilterValueCompletepackages/components/src/custom/index.ts:10→packages/components/src/index.ts:75(export * from './custom').d.tspackages/components/dist/custom/filter-builder.d.ts:399—export declare function isFilterValueComplete(...), reached fromdist/index.d.tsviacustom/index.d.ts:10@object-ui/componentsis alreadyworkspace:*inpackages/app-shell/package.jsonpackages/app-shell/src/views/viewFilterFold.ts:34— a sibling file in the same directory already imports the same symbol from the same entrySo the import costs nothing new: no export added to a published entry, no new dependency edge, no local re-implementation. The added binding rides on the
@object-ui/componentsimport block ObjectView.tsx already had.Reachability of the defect shape — the issue's own open question
The issue asked to measure this first, and explicitly refused to call it a live bug until someone did. Measured: live, not dead-but-misleading code. Two producers, neither of them the converted builder:
This pass is the recovery half for exactly this class of row. Its own docstring says so: "an install already carrying a poisoned row self-heals on the next load rather than needing a
sys_metadatadelete plus a service restart." Pre-conversion builds persisted half-filled ranges throughfoldFilterGroupToSpecRules; the recovery pass self-healed every arity except the one the sibling issue was about.The spec accepts the shape, so any non-builder producer — hand-authored view JSON, an AI-authored metadata app, an import, a migration — can mint it and pass authoring validation. Measured against
ViewFilterRuleSchemafrom@objectstack/spec17.1.0 (the version this tip resolves; the sibling docstrings still say 17.0.0):It counts the two slots and does not ask what is in them. Authoring validation is green on precisely the shape that dies at query time, which leaves this read-path pass as the last guard standing.
Both branches of the pass are converted, because both at-rest shapes reach it: the spec
ViewFilterRuleobject and the legacy runtime triple[field, operator, value]thatpersistViewPatchcopies into an overlay along with the rest of the view.The cited line numbers, re-verified on the current tip
main@7e811687a. #4962's two sites are still converted; one of the issue's three citations had drifted:packages/plugin-list/src/ListView.tsx:414:477packages/app-shell/src/views/viewFilterFold.ts:109:109packages/app-shell/src/views/ObjectView.tsx:330–342:351and:357(pre-patch)After this change the only occurrence of the retired expression in those three files is the one quoted inside the new doc comment.
One premise of the card corrected
The issue's ⭐ note — "it also uses
VALUELESS_FILTER_OPERATORS, not the builder'sVALUELESS_FILTER_BUILDER_OPERATORS… so this surface already keeps its own copy of the operator vocabulary" — is stale, and no reconciliation was needed.viewFilterFold.ts:84builds that set from the shared import (...VALUELESS_FILTER_BUILDER_OPERATORS) plus the canonical spec spellings (is_null) that only this layer sees, andviewFilterFold.emptyValue.test.ts:143already pins the parity. That is exactly the splitisFilterValueComplete's own docstring prescribes — the helper answers the VALUE question, each caller keeps its own operator vocabulary — so the operator half is left untouched deliberately, not overlooked.One shape newly stripped beyond the half-filled array
A
betweenwhose value is a bare scalar ('2024-01-01') used to survive the pass and now does not. That is the same judgement one layer earlier:ViewFilterRuleSchemarefuses it outright (measured above), so the server would too. Pinned as its own case.Measurement — four mutation legs, each restored by a
trapEvery leg is
mutate → assert the mutation landed on disk (injected text AND removed text counted, with pristine counts asserted first) → run → restore. The restore is armed before the mutation, asso a SIGTERM at the foreground cap cannot leave a mutated tree behind for later measurements to silently read.
git status --porcelainwas empty after each leg.(Written that way on purpose: an earlier revision of this body spelled the trap's payload inside angle brackets and GitHub's body sanitizer ate the fragment even inside backticks, leaving a line that read as an empty trap.)
No rebuild step participates:
vitest.config.mts:264aliases@object-ui/componentstopackages/components/src, and the mutated file is app-shell source, so every leg measures source directly.Predicted before running, then measured — the two agreed:
pairblockfalsetruepair-drop block and "still drops an empty scalar and an empty list"VALUELESS_FILTER_OPERATORSearly returnsLeg A is the one the card asked for. Its six reds are the discriminating cases, and they fail on a VALUE rather than on an import — e.g.
Its nine greens are the point: the retired predicate is correct for
scalarandlist, so the wholearities this change must not touchblock passing under leg A is what proves the conversion stayed scoped topair. An assertion that discriminated there would mean the change had reached further.Legs B1/B2/C exist because a control that passes in every world proves nothing. Each is scoped to the same function and turns a different control red, so every one of the 13 assertions about our code is measured capable of failing under at least one leg — and legs A and B1 fail in disjoint directions, which is what separates the pair pin from the controls. The two remaining assertions are facts about
@objectstack/spec, green in every world; they are labelled non-discriminating in the test file itself so nobody later reads them as a pin on our code.Verification
All Vitest runs from the repo root (#3378).
packages/app-shell/src/views/ObjectView.overlayPairValue.test.ts: 15 passed (15).sanitizeViewOverrideinObjectView.tsx; a test can observe it only if that module is in its graph. So a reverse-import BFS from that file over all 3,328 first-party sources (relative specifiers resolved to files,@object-ui/*to the package'ssrcbarrel) yields a 109-module closure containing 58 test files — 19 inpackages/app-shell, 39 inapps/consolereached through the barrel. Anything outside cannot load the changed module.packages/…half:Test Files 19 passed (19) · Tests 222 passed (222)apps/consolehalf:Test Files 39 passed (39) · Tests 365 passed (365)commin both directions is empty for both halves. Worth noting for the next person: theapps/consoleproject does report paths relative to its own root (src/…, notapps/console/src/…), so the comparison normalizes that before diffing.pnpm --filter @object-ui/app-shell type-check→ exit 0, script echoed (tsc --noEmit && tsc -p tsconfig.test.json, so the new test file is type-checked too). The dependency closure was built first (pnpm --workspace-concurrency=2 --filter '@object-ui/app-shell^...' build, exit 0) — a fresh-tree type-check without it fails on the unbuilt closure, not on the diff.pnpm --filter @object-ui/app-shell lint(eslint .over the whole affected package): 0 errors, exit 0 (2,551 pre-existing warnings, unchanged baseline). No narrowing was applied here; the repo-wideturbo run lintacross 46 packages is CI's run.check-control-bytes→✅ OK (scanned 4705 tracked text file(s); skipped 85 binary)check-changeset-presence→✅ 2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)check-changeset-no-major→✅ No changeset declares a 'major' bump.check-phantom-dependencies→ exit 0 (the added import is on an already-declared dependency)check-package-self-import→ exit 0check-lint-coverage→✅ 46/46 packages linted, 0 with outstanding errors(a coverage gate — it validates that every package is reached byturbo run lint, it does not itself run ESLint repo-wide)check-type-check-coverage→✅ 45/46 via type-check, 0 errors outstandingpnpm checkfarm andpnpm lint, which CI runs exactly once either way. Noted, untouched:check-eager-closure-budget(exits 2 locally),check-doc-snippet-types(exits 1),check-published-dist-tooling(belongs to CI).Changeset:
patchfor@object-ui/app-shell.Generated by Claude Code