Skip to content

fix(browse): keep a disabled or stale table-menu item from acting - #1965

Merged
dawsontoth merged 5 commits into
claude/1924-full-scan-dialogfrom
claude/browse-menu-disabled-items
Oct 15, 2026
Merged

dawsontoth merged 5 commits into
claude/1924-full-scan-dialogfrom
claude/browse-menu-disabled-items

Conversation

@dawsontoth

@dawsontoth dawsontoth commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

A badger latches a faded lever so a squirrel pushing it does nothing, beside a fallen signpost

⊙ Problem

Two Browse menu items could act when they shouldn’t. Disabled Cleanup Orphan Blobs still asked to start cleanup on scripted or assistive technology clicks. Drop Table stayed available for a table the map no longer held.

Browse also looked databases and tables up by name in plain objects. A table named constructor counted as present, and a schema database named __proto__ wrote onto Object.prototype.

🤔 Should the by-name reads ride along with the menu fixes?

They came out of the Drop Table check and touch nine Browse files. They would split cleanly into their own PR.

💡 Solution

  • Cleanup Orphan Blobs runs from onSelect, which Radix skips for a disabled item.
  • Drop Table waits for the map to hold the table.
  • Browse reads both maps by name only through own-key helpers.
  • The schema-relationship map is built without prototypes.

⚖️ Alternatives

Three I didn’t take
  • Fix the maps once, where they’re fetched, with null-prototype levels in a select on describe_all. TanStack’s structural sharing copies a changed level into a plain {}, including select results. That requires disabling sharing for a map the roles editor and the chat tools read too. The helpers keep the change inside Browse.
  • A structuralSharing function for the schema-relationship query that keeps the old map when nothing changed. With sharing off, a refetch (on mount, or focus after a minute) recomputes the table view’s relationship memos. Its column header is rebuilt on every render anyway, so I kept false.
  • The same Drop Table check in the table view. Its only mounting site renders <Navigate> when the map loses the table (resolveDatabasesRedirect, run during render in index.tsx), so the check could never fire. DESIGN.md records that the view relies on the redirect.

🔧 Changes

✅ Verification

🤖 Generated with Claude Code by Anthropic Claude Opus 5.5; posted via @dawsontoth.

Related PRs: #1961 overlaps (the base this PR stacks on), #1953 independent (merged; this extends its described-table rule to Drop Table), #1958 independent (both edit DatabaseTableView.tsx, in different hunks), #1955 independent (both edit the databases DESIGN.md, in different sections)
Complexity: medium

Review-Coverage: authored=claude; ran=gemini,cursor-composer,codex,cursor-muse,cursor-grok; adjudicated=domain; declined=cursor-kimi,muse; rounds=11; full=2 @ 7dacf92

Review-Attention: study ~11m (critical: schemaRelationships.ts; decisions: relationship-map-sharing-off, drop-table-gate-asymmetry, do-less-alternative) @ 7dacf92

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces safe database and table lookup utilities to prevent prototype pollution and incorrect lookups when database or table names match Object.prototype properties (such as constructor, toString, or __proto__). It replaces direct property access with these safe lookup functions across various components, modals, and hooks. Additionally, it updates the schema relationship parsing to use prototype-free objects (Object.create(null)) and disables structural sharing for these queries. Finally, it fixes an issue where disabled Radix menu items could still trigger actions by switching from onClick to onSelect. Since there are no review comments, no further feedback is provided.

@dawsontoth
dawsontoth marked this pull request as ready for review October 14, 2026 22:00
@dawsontoth
dawsontoth requested a review from a team as a code owner October 14, 2026 22:00
dawsontoth and others added 5 commits October 15, 2026 09:31
Cleanup Orphan Blobs is disabled while a cleanup is running, but its
handler was on onClick, which Radix runs even for a disabled menu item;
only CSS kept a pointer off it. A screen reader's activation or a
synthetic click therefore still asked to start a second cleanup. The
handler moves to onSelect, which Radix gates on disabled, as #1953 did
for Export CSV. No other disabled-capable menu item under
src/features/instance/databases uses onClick.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The tree and overview menus offered Drop Table whenever the database had
more than one table, even for a table a refetch had just dropped from
the map (another tab or user removed it while its menu was open). Like
Add Records, Import and Export CSV since #1953, it now waits for the map
to hold the table. The menu reads the map's own keys, so a table named
`constructor` no longer finds Object.prototype's and counts as present.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ct member

resolveDatabasesRedirect sends the view away from a database or table
the map no longer holds, and that redirect is what keeps the table
view's toolbar from offering Drop Table for a dropped table. It looked
the names up with a plain property read, so a table named `constructor`
or `toString` found Object.prototype's member, counted as present, and
the view stayed on a table that was gone. It now checks own keys.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Browse looked tables and databases up in the describe_all map and the
schema-relationship map with plain property reads, so a name such as
`constructor` or `toString` found Object.prototype's member. Import Data
then took a new table called `constructor` for an existing one and
said nothing about creating it, and a relationship lookup could be
handed a function where it expected an array.

describedTable.ts adds ownEntry, describedDatabase, describedTable and
declaredRelationships, which read own keys only. The table menus, the
table view, the overview, Import Data, Add Records, the CSV export,
the relationship lookup and resolveDatabasesRedirect now go through
them. The redirect also treats a database entry with no describe as
missing instead of throwing on it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
parseSchemaRelationships filled plain objects keyed by database and
table names read from the instance's component schema files. A database
named `__proto__` therefore wrote its tables onto Object.prototype, and
one named `constructor` wrote them onto Object itself. Both levels of the
map are now null-prototype objects, where those names are ordinary keys,
and the query turns off structural sharing, which would rebuild a changed
level as a plain object on the next refetch. ownEntry also returns
undefined, not null, for an entry that holds nothing.

The databases tree also reads each database's tables through
describedDatabase, so every by-name read of the describe map in Browse
goes through the own-key helpers, as DESIGN.md says. Listing works the
same way: the tree, the overview, the redirect's fallback and both
"last table" checks use describedDatabaseNames and describedTableNames,
which name only entries that have a describe, so they agree with the
lookups on an entry that has none.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dawsontoth
dawsontoth force-pushed the claude/browse-menu-disabled-items branch from 7dacf92 to c419cb9 Compare October 15, 2026 13:33
@dawsontoth
dawsontoth force-pushed the claude/1924-full-scan-dialog branch from d7fe088 to d689810 Compare October 15, 2026 13:33
@dawsontoth
dawsontoth merged commit c419cb9 into claude/1924-full-scan-dialog Oct 15, 2026
2 checks passed
@dawsontoth
dawsontoth deleted the claude/browse-menu-disabled-items branch October 15, 2026 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant