ENG-593 Change from Dev to Admin Panel #415
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a new AdminPanel React component for browsing node schemas and nodes via Supabase; wires it into Settings as a hidden “Secret Admin Panel” opened by Ctrl+Shift+A; comments out the createOrUpdateDiscourseEmbedding call in onload. Also relaxes node schema query to include cache entries with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Settings as Settings Panel
participant AdminPanel as AdminPanel Component
participant SB as Supabase Client
participant Roam as roamAlphaAPI
User->>Settings: Ctrl+Shift+A
Settings->>Settings: setSelectedTabId("secret-admin-panel")
Settings->>AdminPanel: mount (props.onloadArgs)
rect rgb(238,246,255)
note right of AdminPanel: Initialization & data load
AdminPanel->>SB: getLoggedInClient()
SB-->>AdminPanel: client/context
AdminPanel->>SB: getNodeSchemas(spaceId)
SB-->>AdminPanel: [schemas]
AdminPanel->>SB: getNodes(schemaLocalIds)
SB-->>AdminPanel: [nodes]
AdminPanel->>AdminPanel: setState({schemas, nodes})
end
User->>AdminPanel: change schema selection
AdminPanel->>SB: getNodes(newSchema)
SB-->>AdminPanel: [nodes]
AdminPanel->>AdminPanel: setState(nodes)
alt Shift+click content link
AdminPanel->>Roam: open block in right sidebar
else Click with Document present
AdminPanel->>Roam: open page in main window
end
sequenceDiagram
autonumber
participant Ext as Roam Extension
participant Embed as createOrUpdateDiscourseEmbedding
participant Menu as installDiscourseFloatingMenu
Ext->>Embed: (call commented out / SKIPPED)
note right of Ext: TODO: REMOVE AFTER TESTING — embedding step not executed
Ext->>Menu: installDiscourseFloatingMenu()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
apps/roam/src/components/settings/AdminPanel.tsx (3)
74-75: Use array length directly.Minor: more idiomatic and avoids needless Object.keys on arrays.
Apply:
- {Object.keys(this.state.schemas).length > 0 ? ( + {this.state.schemas.length > 0 ? (
80-83: Avoid spreading current state in setState.setState merges; spreading risks stale state.
Apply:
- this.setState({ ...this.state, showingSchema: choice }); - this.getNodes(choice.id).then((nodes) => - this.setState({ ...this.state, nodes }), - ); + this.setState({ showingSchema: choice }); + this.getNodes(choice.id).then((nodes) => + this.setState({ nodes }), + );
77-97: Type the Select for better TS safety.Optional: use a typed Select to catch renderer/selection mismatches.
Add above the component:
const NodeSignatureSelect = Select.ofType<NodeSignature>();Then:
- <Select + <NodeSignatureSelect items={this.state.schemas} ... - > + >apps/roam/src/components/settings/Settings.tsx (2)
78-82: Support macOS (Meta) and avoid repeat keydown triggers.Broaden shortcut compatibility and prevent rapid re-triggering.
Apply:
- if (e.ctrlKey && e.shiftKey && e.key === "A") { + if ((e.ctrlKey || e.metaKey) && e.shiftKey && (e.key === "A" || e.key === "a") && !e.repeat) { e.stopPropagation(); e.preventDefault(); setSelectedTabId("secret-admin-panel"); }
19-19: Lazy-load AdminPanel to keep initial bundle lean.The panel is hidden; defer loading until needed.
Example:
// replace static import // import AdminPanel from "./AdminPanel"; const AdminPanel = React.lazy(() => import("./AdminPanel"));Then wrap the Tab panel:
<React.Suspense fallback={<div className="p-4">Loading…</div>}> <AdminPanel onloadArgs={onloadArgs} /> </React.Suspense>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/roam/src/components/settings/AdminPanel.tsx(1 hunks)apps/roam/src/components/settings/Settings.tsx(3 hunks)apps/roam/src/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API
Applied to files:
apps/roam/src/index.ts
📚 Learning: 2025-06-23T11:49:45.457Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#220
File: apps/roam/src/utils/conceptConversion.ts:11-40
Timestamp: 2025-06-23T11:49:45.457Z
Learning: In the DiscourseGraphs/discourse-graph codebase, direct access to `window.roamAlphaAPI` is the established pattern throughout the codebase. The team prefers to maintain this pattern consistently rather than making piecemeal changes, and plans to address dependency injection as a global refactor when scheduled.
Applied to files:
apps/roam/src/index.ts
🔇 Additional comments (1)
apps/roam/src/components/settings/Settings.tsx (1)
206-210: Approve changes; secret admin panel wiring verified and no stale references found.
| <p> | ||
| Context: <code>{JSON.stringify(this.state.context)}</code> | ||
| </p> | ||
| {Object.keys(this.state.schemas).length > 0 ? ( |
There was a problem hiding this comment.
Avoid leaking secrets: don’t render full Supabase context.
Context may include tokens/URLs. Show only non-sensitive fields.
Apply:
- <p>
- Context: <code>{JSON.stringify(this.state.context)}</code>
- </p>
+ <p>
+ Context:{" "}
+ <code>
+ {JSON.stringify(
+ this.state.context ? { spaceId: this.state.context.spaceId } : null,
+ )}
+ </code>
+ </p>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <p> | |
| Context: <code>{JSON.stringify(this.state.context)}</code> | |
| </p> | |
| {Object.keys(this.state.schemas).length > 0 ? ( | |
| <p> | |
| Context:{" "} | |
| <code> | |
| {JSON.stringify( | |
| this.state.context | |
| ? { spaceId: this.state.context.spaceId } | |
| : null, | |
| )} | |
| </code> | |
| </p> | |
| {Object.keys(this.state.schemas).length > 0 ? ( |
🤖 Prompt for AI Agents
In apps/roam/src/components/settings/AdminPanel.tsx around lines 71-74, the code
renders the entire this.state.context via JSON.stringify which can leak Supabase
tokens/URLs; instead, only display a whitelist of non-sensitive fields (or
explicitly filter out sensitive keys like "apiKey", "token", "access_token",
"refresh_token", "service_role", "url", "endpoint") and/or mask values (e.g.,
show first/last few chars with ****) before rendering; implement a small helper
to pick or sanitize allowed keys from this.state.context and render that
sanitized object (or a simple list of key: maskedValue) rather than
JSON.stringify(this.state.context).
apps/roam/src/index.ts
Outdated
| // TODO: REMOVE AFTER TESTING | ||
| // await createOrUpdateDiscourseEmbedding(onloadArgs.extensionAPI); | ||
| await createOrUpdateDiscourseEmbedding(onloadArgs.extensionAPI); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don’t block extension load with embeddings sync; guard and handle errors.
Running this on every load (and awaiting it) can slow startup and fail in offline/encrypted graphs. Make it dev-only, non-blocking, and resilient.
Apply:
- // TODO: REMOVE AFTER TESTING
- await createOrUpdateDiscourseEmbedding(onloadArgs.extensionAPI);
+ // Dev-only; run non-blocking and don't break load on failure
+ if (process.env.NODE_ENV === "development") {
+ void createOrUpdateDiscourseEmbedding(onloadArgs.extensionAPI).catch((e) =>
+ console.error("createOrUpdateDiscourseEmbedding failed", e),
+ );
+ }Run to confirm there are no other startup-time callers and that the function has timeouts/retries:
🏁 Script executed:
#!/bin/bash
rg -n --type=ts 'createOrUpdateDiscourseEmbedding' -C3Length of output: 2506
Guard extension load from embedding sync in development only
Only startup caller found; wrap in a non-blocking, dev-only call and catch errors:
- // TODO: REMOVE AFTER TESTING
- await createOrUpdateDiscourseEmbedding(onloadArgs.extensionAPI);
+ // Dev-only; run non-blocking and don’t break load on failure
+ if (process.env.NODE_ENV === "development") {
+ void createOrUpdateDiscourseEmbedding(onloadArgs.extensionAPI)
+ .catch((e) => console.error("createOrUpdateDiscourseEmbedding failed", e));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO: REMOVE AFTER TESTING | |
| // await createOrUpdateDiscourseEmbedding(onloadArgs.extensionAPI); | |
| await createOrUpdateDiscourseEmbedding(onloadArgs.extensionAPI); | |
| // Dev-only; run non-blocking and don’t break load on failure | |
| if (process.env.NODE_ENV === "development") { | |
| void createOrUpdateDiscourseEmbedding(onloadArgs.extensionAPI) | |
| .catch((e) => console.error("createOrUpdateDiscourseEmbedding failed", e)); | |
| } |
🤖 Prompt for AI Agents
In apps/roam/src/index.ts around lines 142 to 144, the call to
createOrUpdateDiscourseEmbedding is run unguarded and blocking; change it to run
only in development, non-blocking (do not await), and wrap it in a try/catch to
swallow/log errors so extension load is not blocked. Specifically: check
process.env.NODE_ENV === 'development' (or a suitable dev flag) before invoking,
call the function without awaiting (fire-and-forget) and handle promise
rejection by attaching a .catch that logs the error to the extension logger or
console.
|
Note: To test, you have to uncomment createOrUpdateDiscourseEmbedding in roam/src/index.ts:143 , and run with access to either local or production database. |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Could you please add a loom video walking through the UI/UX changes then re-tag me? Thank you.
maparent
force-pushed
the
eng-593-change-from-dev-to-admin-panel
branch
2 times, most recently
from
805e271 to
bfb894d
Compare
maparent
changed the base branch from
main
to
eng-833-isolate-node-query-functions
maparent
force-pushed
the
eng-593-change-from-dev-to-admin-panel
branch
2 times, most recently
from
5d3dad3 to
cb836b0
Compare
maparent
force-pushed
the
eng-833-isolate-node-query-functions
branch
2 times, most recently
from
d865ad4 to
05df276
Compare
maparent
force-pushed
the
eng-593-change-from-dev-to-admin-panel
branch
from
cb836b0 to
1394ffd
Compare
maparent
force-pushed
the
eng-593-change-from-dev-to-admin-panel
branch
from
1394ffd to
a8343e6
Compare
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
High-level feedback
-
In modern React (since 16.8), functional components with Hooks are the norm for UI. Class components are legacy and generally avoided for new code. (ref) Hooks make data-flow and effects explicit, reduce boilerplate (this, setState merges, lifecycle juggling), and play nicely with newer React features and tooling.
-
Using Hooks here will make the component shorter, easier to test, and less bug-prone (no accidental partial state merges, no stale this.state copies, no componentDidMount branching).
Here's an example of what this code might look like as a functional component with hooks:
https://gist.github.com/mdroidian/88dc865156bef4359a860e9f962203b1
Let's go ahead and update this to functional component with hooks.
Additional notes
- Consider loading/error states; right now failures only
console.error - Consider extracting
<NodesTable />into its own component to keepAdminPanelfocused on orchestration. - There are multiple unused imports, make sure to remove them before submitting the PR
- There are multiple lint warnings, let's try to remove them all. If you have any issues, let me know, or if any specific warnings cannot be removed for some reason, add a comment inline or in GitHub UI.
maparent
force-pushed
the
eng-593-change-from-dev-to-admin-panel
branch
from
a8343e6 to
983f856
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/database/src/lib/queries.ts (1)
99-102: Inconsistent cache filter drops global (spaceId=0) schemas on cache hits.You only include
spaceId === 0after a cache reload, not when serving from cache. This yields inconsistent results depending on cache state.Fix: include
spaceId === 0in the initial cache-hit filter as well.let result = Object.values(NODE_SCHEMA_CACHE) .filter((x) => typeof x === "object") - .filter((x) => x.spaceId === spaceId); + .filter((x) => x.spaceId === spaceId || x.spaceId === 0);
🧹 Nitpick comments (6)
apps/roam/src/components/settings/AdminPanel.tsx (6)
106-124: Initialize supabase/context in parallel and always clear loading in finally.Prevents premature spinner hide and ensures it clears on error.
useEffect(() => { let ignore = false; void (async () => { try { - if (!ignore) { - setContext(await getSupabaseContext()); - } - if (!ignore) { - setSupabase(await getLoggedInClient()); - } + const [ctx, cli] = await Promise.all([getSupabaseContext(), getLoggedInClient()]); + if (!ignore) { + setContext(ctx); + setSupabase(cli); + } } catch (e) { setError((e as Error).message); console.error("AdminPanel init failed", e); + } finally { + if (!ignore) setLoading(false); } })(); return () => { ignore = true; }; }, []);
155-164: Ensure node spinner clears on both success and error.
setLoadingNodes(false)should be in a finally, otherwise it can spin forever on errors.try { setLoadingNodes(true); setNodes( await getNodes({ supabase, spaceId, schemaLocalIds: showingSchema.sourceLocalId, }), ); - setLoadingNodes(false); } catch (e) { setError((e as Error).message); console.error("getNodes failed", e); + } finally { + setLoadingNodes(false); }
194-219: Use array length to check emptiness.
Object.keys(schemas).lengthon arrays is brittle; useschemas.length.- {Object.keys(schemas).length > 0 ? ( + {schemas.length > 0 ? (
17-17: Remove unused import.
OnLoadArgsisn’t used.-import { OnLoadArgs } from "esbuild";
21-22: Redundant key on inside NodeRow.The list key lives on
<NodeRow ... key={node.id} />; the inner<tr key>is unnecessary.- <tr key={node.id}> + <tr>
37-65: Simplify click handler and add basic keyboard accessibility.Avoid nested async IIFE; add role/tabIndex and Enter/Space activation.
- <span - className="rm-page-ref rm-page-ref--link" - onClick={(event) => { - void (async (event) => { - if (event.shiftKey) { - if (node.Content?.source_local_id) { - await window.roamAlphaAPI.ui.rightSidebar.addWindow({ - window: { - // @ts-expect-error TODO: fix this - "block-uid": node.Content.source_local_id, - type: "outline", - }, - }); - } - } else if (node.Content?.Document?.source_local_id) { - await window.roamAlphaAPI.ui.mainWindow.openPage({ - page: { - uid: node.Content.Document.source_local_id, - }, - }); - } - })(event); - }} - > + <span + className="rm-page-ref rm-page-ref--link" + role="link" + tabIndex={0} + onKeyDown={async (e) => { + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + e.currentTarget.click(); + } + }} + onClick={async (event) => { + if (event.shiftKey && node.Content?.source_local_id) { + await window.roamAlphaAPI.ui.rightSidebar.addWindow({ + window: { + // @ts-expect-error TODO: fix this + "block-uid": node.Content.source_local_id, + type: "outline", + }, + }); + } else if (node.Content?.Document?.source_local_id) { + await window.roamAlphaAPI.ui.mainWindow.openPage({ + page: { uid: node.Content.Document.source_local_id }, + }); + } + }} + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/roam/src/components/settings/AdminPanel.tsx(1 hunks)apps/roam/src/components/settings/Settings.tsx(3 hunks)apps/roam/src/index.ts(1 hunks)packages/database/src/lib/queries.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/roam/src/index.ts
- apps/roam/src/components/settings/Settings.tsx
🔇 Additional comments (5)
packages/database/src/lib/queries.ts (1)
123-126: LGTM: reload path correctly includes global schemas.Including
x.spaceId === 0here aligns with the intended behavior. Ensure the cache‑hit path (Lines 99–102) matches this.apps/roam/src/components/settings/AdminPanel.tsx (4)
33-35: LGTM: correct JSX binding for data- attributes.*Attributes now bind expressions rather than literal strings.
126-136: Good: errors are caught and surfaced to UI.Both schema and node fetch effects log errors and set
errorstate. With the loading fixes above, UX will be consistent.Also applies to: 144-173
73-93: Potential performance concerns with large datasets (non-blocking).Rendering full JSON blobs in tables and unpaginated lists can be heavy. Consider pagination/limit in queries, or table virtualization/infinite scroll.
Also applies to: 175-181, 215-216
191-193: Don’t render the full Supabase context (secrets risk).Only masking
spacePasswordstill exposes tokens/URLs. Render a whitelist (e.g., justspaceId) or remove entirely.- <p> - Context:{" "} - <code>{JSON.stringify({ ...context, spacePassword: "****" })}</code> - </p> + <p> + Context: <code>{JSON.stringify(context ? { spaceId: context.spaceId } : null)}</code> + </p>
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Good start on the panel! Could use some UI cleaning for readability, but that can be addressed later.
A few comments, mostly nits. But also take a look at the coderabbit comment and nits.
| type PConcept, | ||
| } from "@repo/database/lib/queries"; | ||
| import { DGSupabaseClient } from "@repo/database/lib/client"; | ||
| import { OnLoadArgs } from "esbuild"; |
| /> | ||
| )} | ||
| > | ||
| <Button text={`display: ${showingSchema.name}`} /> |
There was a problem hiding this comment.
Consider wrapping this in a Label so that display isn't part of the button
<Label>
<Select ...
</Label>It can replace one of the two <div>'s that wrap the <Select>. (the other div can be replaced with <>)
Dev panel has become admin panel (ctrl-shift-A). It shows the supabase Context object, and a table of all nodes of a certain types, starting with schema nodes.
Dev panel has become admin panel (ctrl-shift-A). It shows the supabase Context object, and a table of all nodes of a certain types, starting with schema nodes.
https://linear.app/discourse-graphs/issue/ENG-593/change-from-dev-to-admin-panel
Summary by CodeRabbit
New Features
Changes
Refactor