Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions .changeset/10519-pageview-refresh-in-place.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
---
'@object-ui/app-shell': patch
---

fix(app-shell): a page action refreshes a custom page's data in place instead of remounting the page (objectui#10519)

`PageView` used to key both of its render branches — the ADR-0047 interface list
and the `SchemaRenderer` page — on a counter it bumped after every successful
page-level action that asks for a refresh (an `api` call, a flow, a server
action, an undo, a screen-flow completion). Each such action therefore remounted
the whole page: scroll position, collapsed sections, tab state and in-progress
edits in every embedded block were lost, and every block refetched from scratch.

The counter is gone. `PageView` now answers the console action runtime's
post-action refresh by declaring the change on the data-invalidation bus
(`notifyDataChanged` from `@object-ui/react`, with the unknown-scope
`objectName: '*'`, since a page binds no object). The embedded blocks that read
the bus refetch in place, once each: the object grid, chart, detail view, list
view, kanban, metric, calendar, gallery, timeline, gantt, map, tree, pivot and
data table, every `object-form` layout, `object-master-detail-form` (edit-mode
lines), `report` / `spec-report` over a dataset, a `dashboard`'s dataset table
widget and its `optionsFrom` filter options, an `object-view` drawn as a kanban,
calendar, gallery or timeline, `record:line_items` with an authored parent, and
the `element:number` / `element:repeater` / `element:record_picker` readers. An
`object-form` holding unsaved input keeps it and takes the re-read when it is
saved or reverted (objectui#10572), and a dashboard filter keeps its selected
value. A `kind: 'react'` page is no longer remounted either: its own read
re-runs in place when its effect names the `useDataInvalidation` nonce the page
scope injects (objectui#10887), and a read keyed on `useAdapter` alone is not
re-run by a page action. The page node's `context` is `{ params }` alone:
nothing read the `refreshKey` it also carried. No prop, export or schema key
changes.
2 changes: 1 addition & 1 deletion .changeset/10887-react-page-data-invalidation.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ feat(components): a `kind: 'react'` page's author scope injects `useDataInvalida
A react page reads data through the injected `useAdapter`, in an effect it
writes itself. The scope gave that effect no data-invalidation reader to name,
so the read the react-pages guide taught, keyed on `[adapter]`, re-ran only
when the page was remounted (as `PageView` does after a page action) or its
when the page was remounted (as `PageView` did after a page action) or its
adapter changed.

The scope now injects `useDataInvalidation` from `@object-ui/react` beside
Expand Down
8 changes: 4 additions & 4 deletions .changeset/9673-pageview-drop-context-spread.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ fix(app-shell): `PageView` builds the page node's `context`, it no longer reads
`PageView` spread `(page as any).context` into the `context` it hands `SchemaRenderer`.
`PageSchema` refuses a page-level `context` key, so on every page that parses the
spread added nothing, and the code read as an author channel that no author could use.
The node's `context` is now exactly `{ params, refreshKey }`, built from the route and
the refresh counter, and the `as any` cast at that spot is gone. A stored document
that carries `context` without passing `PageSchema` no longer passes it through.
`context` stays undeclared on `PageSchema` (objectui#9673).
The node's `context` is now built from the route alone, and the `as any` cast at that
spot is gone. A stored document that carries `context` without passing `PageSchema`
no longer passes it through. `context` stays undeclared on `PageSchema`
(objectui#9673).
33 changes: 21 additions & 12 deletions packages/app-shell/src/no-refresh-key-remount.ratchet.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,19 @@
* (`notifyDataChanged` from `@object-ui/react`) and let readers refetch via
* `useDataInvalidation`. See objectui#2269 / DetailView / RecordDetailView.
*
* SCOPE — the RECORD-DETAIL data surfaces #2269 fixed. It is deliberately not
* repo-wide: an explicit user "Refresh this page" affordance
* (`PageView.onRefresh` → `InterfaceListPage key={refreshKey}`) and the Studio
* dev preview harness (`sdui-workbench-preview.tsx`) legitimately remount to
* reset, and are a different concern from "a SAVE silently rebuilt the record
* page under the user". Guarding the fixed surfaces exactly, with no
* allowlist-of-shame, is the honest lock; AGENTS.md Commandment #8 + review
* cover brand-new surfaces.
* SCOPE — the RECORD-DETAIL data surfaces #2269 fixed, plus the custom-page
* host `PageView` (objectui#10519). It is deliberately not repo-wide: the
* Studio dev preview harness (`sdui-workbench-preview.tsx`) legitimately
* remounts to reset, and is a different concern from "a SAVE silently rebuilt
* the page under the user". `PageView.onRefresh` is NOT such an affordance —
* an earlier version of this header called it "an explicit user 'Refresh this
* page' affordance", which was false by a source reading of
* `useConsoleActionRuntime`: `onRefresh` is fed only by the console action
* runtime's post-action refresh (`api`, flow and server-action success, undo,
* screen-flow completion), and `PageView` has no refresh control. objectui#10519
* routed that refresh through the bus and brought the file into scope. Guarding
* the fixed surfaces exactly, with no allowlist-of-shame, is the honest lock;
* AGENTS.md Commandment #8 + review cover brand-new surfaces.
*/

import { describe, it, expect } from 'vitest';
Expand All @@ -46,14 +51,18 @@ const repoRoot = path.resolve(here, '../../..');
const REFRESH_KEY_REMOUNT = /\bkey=\{\s*[^}]*(?:refresh|reload)[^}]*\}/i;

/**
* The record-detail data surfaces #2269 fixed. Path fragments (POSIX) — a
* file is in scope if its repo-relative path contains any of these.
* The record-detail data surfaces #2269 fixed, and the custom-page host
* objectui#10519 fixed. Path fragments (POSIX) — a file is in scope if its
* repo-relative path contains any of these.
*/
const IN_SCOPE = [
'packages/plugin-detail/src/',
'packages/app-shell/src/views/RecordDetailView',
'packages/app-shell/src/views/RelatedRecordActionsBridge',
'packages/app-shell/src/console/AppContent',
// objectui#10519: the page host declares a page action's change on the bus;
// its two render branches are keyed on identity, never on a refresh counter.
'packages/app-shell/src/views/PageView',
];

function collectSourceFiles(): string[] {
Expand Down Expand Up @@ -96,13 +105,13 @@ function collectSourceFiles(): string[] {
}

describe('objectui#2269 — no refetch-by-remount ratchet', () => {
it('finds the in-scope record-detail surfaces (guards against a broken scan path)', () => {
it('finds the in-scope record-detail surfaces and the PageView page host (guards against a broken scan path)', () => {
// plugin-detail alone has dozens of source files; if this drops the
// scope globs have gone stale and the ratchet would silently pass.
expect(collectSourceFiles().length).toBeGreaterThan(20);
});

it('has zero `key={…refresh/reload…}` remount sites in the record-detail surfaces', () => {
it('has zero `key={…refresh/reload…}` remount sites in the record-detail surfaces or the PageView page host', () => {
const offenders: string[] = [];
for (const file of collectSourceFiles()) {
const src = readFileSync(file, 'utf8');
Expand Down
32 changes: 23 additions & 9 deletions packages/app-shell/src/views/PageView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,8 @@
* embedding the heavyweight page canvas in the runtime.
*/

import { useState } from 'react';
import { useParams, useSearchParams, useNavigate, useLocation } from 'react-router-dom';
import { SchemaRenderer, useAdapter } from '@object-ui/react';
import { SchemaRenderer, notifyDataChanged, useAdapter } from '@object-ui/react';
import { Empty, EmptyTitle, EmptyDescription, Spinner } from '@object-ui/components';
import { FileText, Pencil } from 'lucide-react';
import { useObjectTranslation } from '@object-ui/i18n';
Expand All @@ -24,6 +23,25 @@ import { ConsoleActionRuntimeProvider } from '../hooks/useConsoleActionRuntime.j
import { useCanAuthorMetadata } from '../hooks/useCanAuthorMetadata.js';
import { InterfaceListPage } from './InterfaceListPage.js';

/**
* After a successful page-level action, declare the change on the
* data-invalidation bus so every embedded block that reads it refetches IN
* PLACE (AGENTS.md #8's corollary: refresh data, don't rebuild UI). A page
* binds no object and the runtime's refresh carries none, so the scope is the
* bus's documented unknown-scope value.
*
* This host used to bump a counter into the `key` of both render branches
* below, which remounted the whole page on every page action: scroll, collapsed
* sections and in-progress edits in every block went with it, and every block
* refetched from scratch (objectui#10519). Both branches are now keyed on
* identity; `no-refresh-key-remount.ratchet` holds this file in scope.
* Module-level so its identity is stable across renders (the runtime names it
* in dependency lists).
*/
function declarePageDataChanged(): void {
notifyDataChanged({ objectName: '*' });
}

export function PageView() {
const { t } = useObjectTranslation();
const { pageName } = useParams<{ pageName: string }>();
Expand All @@ -48,9 +66,6 @@ export function PageView() {
// container instead of by load order.
const { app: activeApp } = useExpressionContext();
const dataSource = useAdapter();
// Bumped after a successful page action so embedded data (lists, etc.)
// re-fetch. Threaded into the page context AND used to remount the renderer.
const [refreshKey, setRefreshKey] = useState(0);
const page = preferLocal(pages as any[], pageName, (activeApp as any)?._packageId);

if (!page) {
Expand Down Expand Up @@ -103,7 +118,7 @@ export function PageView() {
<ConsoleActionRuntimeProvider
dataSource={dataSource}
objects={objects}
onRefresh={() => setRefreshKey((k) => k + 1)}
onRefresh={declarePageDataChanged}
>
<div className="flex flex-row h-full w-full overflow-hidden relative">
<div className="flex-1 overflow-auto h-full relative">
Expand All @@ -122,10 +137,9 @@ export function PageView() {
{(page as any).interfaceConfig?.source ? (
// ADR-0047 interface mode: the page binds a source view into a
// curated list surface — rendered directly, not via regions.
<InterfaceListPage key={refreshKey} page={page} reserveEditAffordance={canEditInStudio} />
<InterfaceListPage page={page} reserveEditAffordance={canEditInStudio} />
) : (
<SchemaRenderer
key={refreshKey}
schema={{
...page,
// `type` stays the SchemaNode discriminator ComponentRegistry
Expand Down Expand Up @@ -163,7 +177,7 @@ export function PageView() {
// refuses a page-level `context` key, so no parsed page can
// carry one (objectui#9673). Written after `...page`, it also
// overrides whatever an unparsed document smuggled in.
context: { params, refreshKey },
context: { params },
}}
/>
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
* never carries one. PageView used to spread `(page as any).context` into the
* node anyway: a no-op on every parsed page, and a channel that read as
* author-supplied page context that no author could supply. Ruled "remove"
* (comment 5811058824): the node's `context` is exactly `{ params, refreshKey }`.
* (comment 5811058824): the node's `context` is exactly `{ params }` — the
* `refreshKey` it also carried when this was ruled left with objectui#10519,
* which measured that no block read it.
*
* The second case is the discriminating one: a document that never passed
* `PageSchema` and sneaks `context` in through a cast must not leak it into the
Expand Down Expand Up @@ -96,7 +98,7 @@ function writeFor(doc: Record<string, unknown>, query: string): Record<string, a
const PARSED_PAGE = { name: PAGE_NAME, label: 'A Page', type: 'app' };

describe('objectui#9673 — PageView builds the node context, it never reads one off the page', () => {
it('hands SchemaRenderer a context of exactly { params, refreshKey } for a page that parses', () => {
it('hands SchemaRenderer a context of exactly { params } for a page that parses', () => {
// Control: the fixture is a page PageSchema accepts, so this is the case
// every real author is in.
expect(PageSchema.safeParse(PARSED_PAGE).success).toBe(true);
Expand All @@ -105,7 +107,7 @@ describe('objectui#9673 — PageView builds the node context, it never reads one

// Firing control: absence means the harness stopped reaching SchemaRenderer.
expect(schema, 'PageView rendered no schema at all; this probe measured nothing').toBeDefined();
expect(schema!.context).toEqual({ params: { account: '42', tab: 'notes' }, refreshKey: 0 });
expect(schema!.context).toEqual({ params: { account: '42', tab: 'notes' } });
});

it('a document that sneaks `context` in past PageSchema does not leak it into the node', () => {
Expand All @@ -121,6 +123,6 @@ describe('objectui#9673 — PageView builds the node context, it never reads one
schema!.context,
'the node context must be built from the route alone; a `context` key on the stored page ' +
'is one PageSchema refuses and must not reach SchemaRenderer (objectui#9673).',
).toEqual({ params: { account: '42' }, refreshKey: 0 });
).toEqual({ params: { account: '42' } });
});
});
Loading
Loading