Uh oh!
There was an error while loading. Please reload this page.
fix(app-shell,plugin-detail,react): offer approval recall to the submitter only - #6485
Merged
Merged
Conversation
…itter only The record page's approval band lit its Recall button for every reader of a pending record. Recall is authorized server-side on submitter identity and refused to everyone else, so a non-submitter was handed a lever whose click could only produce a failure toast — a writability-feedback mismatch, not a permission question. Record state was never at risk. The button's only gate was `dataSource.cancelPendingApproval`, which asks whether the ADAPTER can recall, not whether the VIEWER may. Identity now joins it, threaded the way the rest of the band's signals already travel: the host resolves it and passes `InlineEditProvider.approvalIsSubmitter`, so the renderer stays DataSource-agnostic. The signal is tri-state and the third state is load-bearing: `undefined` — a host that resolves no approval identity — renders exactly as before, so no host silently loses its submitter's only unlock lever. Withdrawn rather than disabled-with-reason, matching the sibling submitter levers (the approvals panel's Remind, the declared `approval_recall` action's `visible` predicate). `isSubmitterOf` is now the one derivation of who submitted — server-resolved `viewer.is_submitter` first, id comparison as the pre-framework#3310 fallback, joined with `??` so a server `false` is believed rather than re-litigated. The Remind gate reads the same answer; its behaviour is unchanged. No permission changes: `canEdit` and the approval lock are untouched, nothing downstream reads this as authorization, and there is deliberately no admin carve-out. The untranslated refusal text seen in the same report is tracked separately on objectstack#11993 and is out of scope here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
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-support-ai
marked this pull request as ready for review
August 26, 2026 04:55
This was referenced Aug 26, 2026
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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#6464
All evidence below was produced on
977b301f1, which is this branch's final commit — the tree every run measured.The defect, and where it actually lives
The card says withdraw; the code says
cancelApproval/ Recall. Censusing the shape rather than the word also moved the site: the button is not inRecordApprovalsPanel.tsx(that file has only the Remind lever). It is the approval band's recall button inpackages/plugin-detail/src/DetailView.tsx, and its only gate was— a question about the adapter ("can this DataSource recall at all"), never about the viewer. So every reader of a pending record got a lit button, which matches the field report exactly. The English text the reporter saw concatenated after a localized prefix is thrown by
packages/data-objectstack/src/index.ts(NO_PENDING_REQUEST, 404) when the request list comes back empty for a non-submitter.RecordApprovalsPanel.tsx:266-274was still the right precedent — its Remind gate states the source order — but it is twenty lines from a different control in a different package.The fix
Identity now joins the adapter gate, threaded the way every other band signal already travels: the host resolves it, the renderer stays DataSource-agnostic.
@object-ui/reactInlineEditProvideracceptsapprovalIsSubmitter, surfaced onInlineEditContextValue@object-ui/plugin-detailfalse@object-ui/app-shellRecordDetailViewresolves the verdict from its existing approvals read and threads itTri-state, and the third state is load-bearing.
trueoffers recall,falsewithdraws it, andundefined— a host that resolves no approval identity — renders exactly as before. Omission preserving prior behaviour mirrors howapprovalPendingfalls back tolocked. Defaulting the unknown case to hide would have traded a cosmetic defect for a functional loss: every host whose band runs off the record'sapproval_statusmirror alone would silently lose its submitter's only way to unlock their own record.Hide, not disable-with-reason. The card offered either; this picks hide, on three grounds. For a non-submitter this control is never actionable on any pending record, so a permanently disabled button is standing clutter rather than a lesson. Both sibling submitter levers already hide — the approvals panel's Remind button, and the declared
approval_recallaction'svisiblepredicate — so disabling here would make one of three identical levers behave differently. And nothing is withheld by hiding it: the band, its quorum tally and the approvals timeline still tell a non-submitter exactly what state the record is in.This changes no permission.
canEditand the approval lock are untouched, the recall endpoint remains the sole authority, and nothing downstream readsapprovalIsSubmitteras an authorization verdict — it decides who is shown the entry, not who may act. There is deliberately no admin carve-out (the reporter ruled that case out). The refusal-message i18n half is not addressed here; out of scope: it stays onobjectstack#11993.One derivation, not two
isSubmitterOfis now the single answer to "did this viewer submit this approval" — server-resolvedviewer.is_submitterfirst, id comparison as the pre-framework#3310fallback, joined with??so a server that resolvedfalseis believed rather than re-litigated client-side. The Remind gate, which carried that expression inline, now reads the same function; its behaviour is unchanged and its existing suites are run below as controls. Two copies would have been two definitions of who submitted — the hazardutils/approverIdentityalready exists to prevent on the display side.Reverse verification — both halves, prediction stated before each run
Direction predicted before each run, and printed into the log ahead of it. Restore proven by blob-hash equality against
HEADplus an emptygit diff HEAD, under anEXIT/INT/TERMtrap holding absolute paths; mutation proven by grepping the injected and the deleted text, never a diffstat.No rebuild is needed for either leg, and that is a property of this repo, not an omission.
vitest.config.mtsaliases@object-ui/reactand@object-ui/plugin-detailto their ownsrc/, and both suites import the mutated modules relatively (../DetailView,./RecordDetailView). Nothing here resolves a dependency through itsdist/exports, so a rebuild would change nothing these runs read.Leg A — ablate the band's gate (
DetailView.tsx→ basea672ae917). On-disk proof: deleted textinline?.approvalIsSubmitter !== falsecount 0, restored bare gate count 1, guard comment count 0.The 3 red are exactly the 3 predicted discriminating cells (non-submitter pending, non-submitter locked, the re-render walk). The 4 control cells in the same file stayed green — submitter keeps it,
undefinedunchanged, adapter-cannot-recall — as did all ofRecordDetailView.approvalRecallGate,isSubmitterOf,DetailView.approvalBandandRecordApprovalsPanel.Leg B — ablate the host threading (
RecordDetailView.tsx→ base). On-disk proof: deleted prop 0, deletedisSubmitterOf(...)call 0, deleted comment 0, injected base import 1.The 4 red are the four resolved-verdict cells. The fifth test in that same file — "threads
undefined— notfalse— when there is no request to consult" — stayed green, which is the cell that proves this leg isolated the seam rather than flipping everything.Both restores:
worktree blob == HEAD blob,git diff HEADempty,git status --porcelainempty.Tests
Every absence assertion is paired with a positive one in the same query (
role="status"band +role="progressbar"tally), so "the button is not in the document" is a measurement rather than a blank page. The re-render walk (false → true → false → undefined) doubles as the isolation pin: each assertion has already seen the previous verdict on the same mount.New:
DetailView.approvalRecallGate.test.tsx(7),useRecordApprovals.isSubmitterOf.test.ts(9),RecordDetailView.approvalRecallGate.test.tsx(5). Controls run alongside:DetailView.approvalBand,InlineEditContext,RecordApprovalsPanel×3,RecordDetailView.approvalDeclaredActions,approvalOverride.type-checkgreen in all three packages (TYPECHECK_EXITcaptured directly, not through a pipe; script names echoed, so no zero-match false green). Both pin files confirmed in the tsc program viatsc -p tsconfig.test.json --listFiles— plugin-detail 1, app-shell 2.Clause ② — is the published surface widened? Yes, in one package. Stated in as many words:
The entry-file diff (
git diff --stat -- '**/src/index.ts') is empty, and that does not settle it.@object-ui/react— WIDENED.src/index.tsdoesexport * from './context/index.js'→export * from './InlineEditContext.js', soInlineEditContextValueandInlineEditProviderPropsare already exported by name, and this PR adds an optional member to each. Confirmed in the built artefact:packages/react/dist/context/InlineEditContext.d.tscarriesapprovalIsSubmitter?: boolean;at both sites, reachable fromdist/index.d.ts. Additive and optional — no existing consumer stops compiling — but it is a widened surface, and the changeset marks this packageminorfor exactly that reason.@object-ui/app-shell— NOT widened.isSubmitterOfis internal. Two named-only hops (src/index.ts←hooks/index.js←useRecordApprovals.js), neither naming it; zeroexport *in either barrel;package.json#exportspublishes only.. This is settled from source, not fromdist/—packages/app-shell/distdoes not exist in this worktree, so adistgrep here would have proven nothing.@object-ui/plugin-detail— NOT widened. A JSX condition and comments; no type declaration changed,DetailViewPropsuntouched.Gates
Verdict lines as each gate printed them; exit codes captured before any pipe.
check:control-bytes✅ check-control-bytes: OK (scanned 5360 tracked text file(s); skipped 85 binary).check:vi-mock-specifiers✅ check-vi-mock-specifiers: OK (… 451 carry a mock; 701 relative specifier(s) resolved …)check:phantom-deps✅ Every in-scope import is declared by the package that publishes it.check:self-import✅ No package names itself inside its own src/.check:entry-guard✓ check:entry-guard: 50 scripts/ file(s) — no entry guard outside the baselinecheck:i18n-keysEvery in-scope call-site key resolves against the en pack (2856 keys) …✅ 5 source file(s) of 3 released package(s) changed, and this change declares 1 changeset(s)✅ No changeset declares a 'major' bump.Two gates are NOT MEASURED locally — a missing prerequisite, read as neither green nor red:
check:readme-exportsexits 1 with❌ 69 self-import(s) could not be judged, every entry carrying the gate's own remedy "runpnpm buildfirst". Only dependency closures were built here, so seven packages have nodist/index.d.ts. None of the 69 namesplugin-detail— the one README this PR touches — whosedist/index.d.tsdoes exist (built as a dependency of app-shell), so its self-import at README line 58 was judged and passed. The added bullet contributes 0 imports and 0 code fences, so it cannot move this gate either way.check:eager-closureexits 2 with❌ No eager-closure report at apps/console/dist/eager-closure.json … This is a broken gauge, not 3 budgets that all passed.It needs a console build. Both are CI's to run against a full build.A raw control-character scan was also run outside the gate over all ten changed files (
grep -naP '[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]'): no hits.Lint — narrowed, and the narrowing is measured
Not a repo-wide
pnpm lint; eslint was run over the changed.ts/.tsxset. Three pieces of evidence, so this is a measurement rather than a skip:eslint.config.jsscopes rules tofiles: ['**/*.{ts,tsx}'], so the two Markdown files in this diff are outside the linted population entirely.--format json:8 files linted,ESLINT_EXIT=0, 0 errors, 191 warnings.tseslint.configs.recommended(notrecommendedTypeChecked), and zero occurrences ofproject/projectServiceanywhere ineslint.config.js. Every file's verdict is computed from that file plus static config, so a diff confined to these files cannot change the verdict of any file it did not touch.Intersecting eslint's reported line numbers with the lines this diff adds: on the 5 modified files, 183 pre-existing warnings, 0 of them on a line this diff added. The remaining 8 are on the new test files, all
@typescript-eslint/no-explicit-anyonfetch/ metadata / DataSource stub casts — the house pattern of the sibling harness (RecordDetailView.approvalDeclaredActions.test.tsx, 7 such casts).Notes for review
objectstack#11993: the mixed-language refusal text may not be server-emitted at all."No pending approval request found for this record"is thrown client-side bypackages/data-objectstack/src/index.tswhen the request lookup returns no visible pending row — the shape a non-submitter gets. That card is scopeddomain:serviceson the premise the server emits it; the string is at least also present on this side. Not touched here.Generated by Claude Code