Skip to content

[ENG-1545] Switch positions of node type and node title fields #907

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
trangdoan982 wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[ENG-1545] Switch positions of node type and node title fields #907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5876cc
[ENG-1545] Switch positions of node type and node title fields
trangdoan982 Mar 22, 2026
f3d4630
lint and remove unnecessary change
trangdoan982 Mar 27, 2026
91cec86
small fixes
trangdoan982 Mar 27, 2026
91e1f57
address pr comment
trangdoan982 Mar 28, 2026
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
125 changes: 92 additions & 33 deletions apps/roam/src/components/ModifyNodeDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,13 +109,16 @@ const ModifyNodeDialog = ({
: allNodes.filter(excludeDefaultNodes);
}, [includeDefaultNodes]);

const [selectedNodeType, setSelectedNodeType] = useState(() => {
const [selectedNodeType, setSelectedNodeType] = useState<
Copy link
Copy Markdown
Member

@mdroidian mdroidian Apr 9, 2026

Choose a reason for hiding this comment

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

We have an explicit type for this: DiscourseNode. Let's use that to be clear and direct.

(typeof discourseNodes)[number] | null
Copy link
Copy Markdown
Member Author

@trangdoan982 trangdoan982 Mar 27, 2026

Choose a reason for hiding this comment

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

ENG-1554: Add support for empty Node Type dropdown

>(() => {
if (!nodeType) return null;
Copy link
Copy Markdown
Member

@mdroidian mdroidian Apr 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Let's make nodeType optional if we are doing this.

const node = discourseNodes.find((n) => n.type === nodeType);
return node || discourseNodes[0];
return node || null;
});

const nodeFormat = useMemo(() => {
return selectedNodeType.format || "";
return selectedNodeType?.format || "";
}, [selectedNodeType]);

const referencedNode = useMemo(() => {
Expand Down Expand Up @@ -160,6 +163,39 @@ const ModifyNodeDialog = ({
if (contentRequestIdRef.current === req && alive) {
setOptions((prev) => ({ ...prev, content: results }));
}
} else {
// Query all discourse node types in parallel
const allResults = await Promise.all(
Copy link
Copy Markdown
Member

@mdroidian mdroidian Apr 9, 2026

Choose a reason for hiding this comment

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

discourse-graphs has 20+ nodes, so this would send 20+ queries and hammer the server + take a very long time. Let's replace this with the same pattern used in getAllDiscourseNodesSince.ts:75-105 (extract into a reusable function called getAllDiscourseNodeInstances or similar

discourseNodes.map(async (node) => {
const conditionUid = window.roamAlphaAPI.util.generateUID();
const results = await fireQuery({
returnNode: "node",
selections: [],
conditions: [
{
source: "node",
relation: "is a",
target: node.type,
uid: conditionUid,
type: "clause",
},
],
});
return results.map((r) => ({
...r,
discourseNodeType: node.type,
}));
}),
);
const seen = new Set<string>();
const deduped = allResults.flat().filter((r) => {
if (seen.has(r.uid)) return false;
seen.add(r.uid);
return true;
});
if (contentRequestIdRef.current === req && alive) {
setOptions((prev) => ({ ...prev, content: deduped }));
}
}
} catch (error) {
if (contentRequestIdRef.current === req && alive) {
Expand Down Expand Up @@ -224,11 +260,25 @@ const ModifyNodeDialog = ({
alive = false;
refAlive = false;
};
}, [selectedNodeType, referencedNode]);

const setValue = useCallback((r: Result) => {
setContent(r);
}, []);
}, [selectedNodeType, referencedNode, discourseNodes]);

const setValue = useCallback(
(r: Result) => {
setContent(r);
if (!selectedNodeType && r.uid) {
const detectedType = (r as Record<string, unknown>)
Copy link
Copy Markdown
Member

@mdroidian mdroidian Apr 9, 2026

Choose a reason for hiding this comment

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

Is this assertion necessary? It's already a Result

.discourseNodeType as string | undefined;
Copy link
Copy Markdown
Member

@mdroidian mdroidian Apr 9, 2026

Choose a reason for hiding this comment

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

Is this assertion necessary?

if (detectedType) {
Copy link
Copy Markdown
Member Author

@trangdoan982 trangdoan982 Mar 27, 2026

Choose a reason for hiding this comment

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

ENG-1555: Lock Node Type dropdown when node is selected

const nt = discourseNodes.find((n) => n.type === detectedType);
if (nt) {
setSelectedNodeType(nt);
setError("");
}
}
}
},
[selectedNodeType, discourseNodes],
);

const setReferencedNodeValueCallback = useCallback((r: Result) => {
setReferencedNodeValue(r);
Expand Down Expand Up @@ -304,9 +354,13 @@ const ModifyNodeDialog = ({

const onSubmit = async () => {
if (!content.text.trim()) return;
if (!selectedNodeType && !isContentLocked) {
setError("Please select a node type");
return;
}
posthog.capture("Modify Node Dialog: Submit Triggered", {
mode,
nodeType: selectedNodeType.type,
nodeType: selectedNodeType?.type,
});
try {
if (mode === "create") {
Expand All @@ -326,7 +380,7 @@ const ModifyNodeDialog = ({
await addImageToPage({
pageUid,
imageUrl,
configPageUid: selectedNodeType.type,
configPageUid: selectedNodeType?.type || "",
extensionAPI,
});
}
Expand Down Expand Up @@ -373,7 +427,7 @@ const ModifyNodeDialog = ({
} else {
formattedTitle = await getNewDiscourseNodeText({
text: content.text.trim(),
nodeType: selectedNodeType.type,
nodeType: selectedNodeType!.type,
blockUid: sourceBlockUid,
});
}
Expand All @@ -384,7 +438,7 @@ const ModifyNodeDialog = ({
// Create new discourse node
const newPageUid = await createDiscourseNode({
text: formattedTitle,
configPageUid: selectedNodeType.type,
configPageUid: selectedNodeType!.type,
extensionAPI,
imageUrl,
});
Expand Down Expand Up @@ -505,6 +559,26 @@ const ModifyNodeDialog = ({
style={{ pointerEvents: "all" }}
>
<div className={`${Classes.DIALOG_BODY} flex flex-col gap-4`}>
{/* Content Input */}
Copy link
Copy Markdown
Member Author

@trangdoan982 trangdoan982 Mar 27, 2026

Choose a reason for hiding this comment

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

ENG-1556: Move Node Type dropdown below content input

<div className="w-full">
<Label>Content</Label>
Copy link
Copy Markdown
Member

@mdroidian mdroidian Apr 9, 2026

Choose a reason for hiding this comment

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

This was pre-exisitng, but let's wrap the FuzzySelectInput with Label to match the styling of the Node Type Selector. It's counter intuitive, but matches blueprint

<FuzzySelectInput
value={content}
setValue={setValue}
options={options.content}
placeholder={
loading
? "..."
: selectedNodeType
? `Enter a ${selectedNodeType.text.toLowerCase()} ...`
: "Search all nodes..."
}
mode={mode}
isLocked={isContentLocked}
autoFocus={!isContentLocked}
/>
</div>

{/* Node Type Selector */}
<div className="flex w-full">
<Label autoFocus={false}>
Copy link
Copy Markdown
Member

@mdroidian mdroidian Apr 9, 2026

Choose a reason for hiding this comment

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

When tabbing to Node Type, it isn't showing the active state so a user doesn't know if they are on this field.

Image

Expand All @@ -514,43 +588,28 @@ const ModifyNodeDialog = ({
transformItem={(t) =>
discourseNodes.find((n) => n.type === t)?.text || t
}
activeItem={selectedNodeType.type}
activeItem={selectedNodeType?.type ?? null}
onItemSelect={(t) => {
const nt = discourseNodes.find((n) => n.type === t);
if (nt) {
setSelectedNodeType(nt);
setReferencedNodeValue({ text: "", uid: "" });
setError("");
}
}}
disabled={mode === "edit" || disableNodeTypeChange}
disabled={
mode === "edit" || disableNodeTypeChange || isContentLocked
}
popoverProps={{ openOnTargetFocus: false }}
className={
mode === "edit" || disableNodeTypeChange
mode === "edit" || disableNodeTypeChange || isContentLocked
? "cursor-not-allowed opacity-50"
: ""
}
/>
</Label>
</div>

{/* Content Input */}
<div className="w-full">
<Label>Content</Label>
<FuzzySelectInput
value={content}
setValue={setValue}
options={options.content}
placeholder={
loading
? "..."
: `Enter a ${selectedNodeType.text.toLowerCase()} ...`
}
mode={mode}
isLocked={isContentLocked}
autoFocus={!isContentLocked}
/>
</div>

{/* Referenced Node Input */}
{referencedNode && !isContentLocked && mode === "create" && (
<div className="w-full">
Expand Down
12 changes: 1 addition & 11 deletions apps/roam/src/utils/registerCommandPaletteCommands.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,19 +189,9 @@ export const registerCommandPaletteCommands = (onloadArgs: OnloadArgs) => {

const selectionStart = uid ? getSelectionStartForBlock(uid) : 0;

const defaultNodeType =
getDiscourseNodes().filter(excludeDefaultNodes)[0]?.type;
if (!defaultNodeType) {
renderToast({
id: "create-discourse-node-command-no-types",
content: "No discourse node types found in settings.",
});
return;
}

renderModifyNodeDialog({
mode: "create",
nodeType: defaultNodeType,
nodeType: "",
Copy link
Copy Markdown
Member Author

@trangdoan982 trangdoan982 Mar 27, 2026

Choose a reason for hiding this comment

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

ENG-1554: Add support for empty Node Type dropdown

initialValue: { text: "", uid: "" },
extensionAPI,
onSuccess: async (result) => {
Expand Down
Loading