feat: add nXus Editor mini-app and improve gateway proxy#61
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new nxus-editor app: UI components (outline, fields, breadcrumbs, bullets), Zustand store, optimistic sync hook, server endpoints for outline CRUD/reorder/reparent, routing/app shell, build/test configs, styles, seed data, tests (unit + e2e), and gateway/dev integration under /editor. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Outline UI
participant Store as Zustand Store
participant Hook as useOutlineSync
participant Server as Outline Server
participant DB as Database
User->>UI: Create node after X
UI->>Store: createNodeAfter(afterId) (optimistic temp ID)
Store->>Hook: createNodeAfter(tempId)
Hook->>Server: createNodeServerFn({ parentId, content, order })
Server->>DB: INSERT node -> serverId
Server-->>Hook: serverId
Hook->>Store: replace tempId with serverId
Store->>UI: re-render with server ID
sequenceDiagram
actor User
participant UI as NodeContent Editor
participant Store as Zustand Store
participant Hook as useOutlineSync
participant Server as Outline Server
participant DB as Database
User->>UI: Type text
UI->>Store: updateNodeContent(nodeId, content) (optimistic)
Store->>Hook: schedule debounced save
Note over Hook: debounce 500ms
Hook->>Server: updateNodeContentServerFn({ nodeId, content })
Server->>DB: UPDATE node
Server-->>Hook: ack (fire & forget)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (9)
apps/nxus-editor/src/routes/__root.tsx (2)
131-131: Prefer a named props interface forRootDocument.The inline prop object type is fine mechanically, but it drifts from the repo's component typing convention and gets harder to extend once shell props grow.
As per coding guidelines, "Define React component props with TypeScript interfaces".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/routes/__root.tsx` at line 131, The RootDocument component uses an inline prop type; define a named interface (e.g., interface RootDocumentProps { children: React.ReactNode }) and replace the inline type in the RootDocument signature with that interface (function RootDocument(props: RootDocumentProps) or destructure as function RootDocument({ children }: RootDocumentProps)), ensuring the prop type follows the repo convention and is easy to extend later; update any imports/exports or usages if they rely on the previous inline signature.
64-83: Validate thenxus-themepayload before using it.
localStorageis external input, butJSON.parse(stored).stateassumes the shape and trusts arbitrarypalette/colorModevalues. Parse it with a small Zod schema so stale or malformed theme data falls back cleanly instead of partially mutatingdocumentElement.As per coding guidelines, "Use Zod schemas with parse() for validating external data instead of assuming validity" and "Validate external inputs using Zod schemas before processing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/routes/__root.tsx` around lines 64 - 83, The applyTheme function currently trusts localStorage.getItem('nxus-theme') and uses JSON.parse(stored).state directly, which can lead to partial/malformed mutations of document.documentElement; create a small Zod schema (e.g., { state: z.object({ colorMode: z.enum(['light','dark']).default('dark'), palette: z.string().refine(p=>ALL_PALETTES.includes(p) || p === 'default').default('default') }).optional().default({ state: { colorMode: 'dark', palette: 'default' } }) }) and use schema.parse(parsedJson) before touching classes, then compute colorMode/palette from the validated result and only then remove/add classes on document.documentElement (keep using ALL_PALETTES and preserve default fallbacks).apps/nxus-editor/src/components/outline/bullet.tsx (1)
70-73: Remove unusedisLastprop from TreeLineProps.
isLastis declared in the interface but not used in theTreeLinecomponent implementation.♻️ Proposed fix
interface TreeLineProps { depth: number - isLast: boolean } -export function TreeLine({ depth }: TreeLineProps) { +export function TreeLine({ depth }: TreeLineProps) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/components/outline/bullet.tsx` around lines 70 - 73, TreeLineProps declares an unused prop isLast; remove isLast from the TreeLineProps interface and any related references so the TreeLine component's props only include depth (update the TreeLineProps definition and any destructuring or type annotations in the TreeLine component to match), ensuring no other code expects isLast from TreeLine.apps/nxus-editor/src/components/outline/node-block.tsx (2)
179-183: Memoize sorted children to avoid recomputation on every render.
sortedChildrenis recomputed on each render. Since sorting depends onnode.childrenandnodes, wrap it inuseMemofor better performance, especially with large outlines.♻️ Proposed fix
+import { memo, useCallback, useMemo } from 'react' // ... - const sortedChildren = [...node.children].sort((a, b) => { - const na = nodes.get(a) - const nb = nodes.get(b) - return (na?.order ?? '').localeCompare(nb?.order ?? '') - }) + const sortedChildren = useMemo( + () => + [...node.children].sort((a, b) => { + const na = nodes.get(a) + const nb = nodes.get(b) + return (na?.order ?? '').localeCompare(nb?.order ?? '') + }), + [node.children, nodes], + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/components/outline/node-block.tsx` around lines 179 - 183, sortedChildren is being recomputed every render; wrap the sort logic in React's useMemo so it only recalculates when node.children or nodes change. Replace the current const sortedChildren = [...node.children].sort(...) with a useMemo that returns the sorted array and lists node.children and nodes (or their relevant identity/stable references) as dependencies; reference the sortedChildren variable, node.children, and nodes in your change and ensure the comparator stays identical inside the memo.
16-33: Consider consolidating store selectors to reduce subscription overhead.Each
useOutlineStorecall creates a separate subscription. With 14 individual selectors, the component may re-render more often than necessary. Consider using a single selector that returns an object with all needed values, or use Zustand'suseShallowfor object selectors.♻️ Example consolidation pattern
+import { useShallow } from 'zustand/react/shallow' + +const { + node, + activeNodeId, + selectedNodeId, + cursorPosition, + nodes, +} = useOutlineStore( + useShallow((s) => ({ + node: s.nodes.get(nodeId), + activeNodeId: s.activeNodeId, + selectedNodeId: s.selectedNodeId, + cursorPosition: s.cursorPosition, + nodes: s.nodes, + })), +) + +const actions = useOutlineStore( + useShallow((s) => ({ + activateNode: s.activateNode, + toggleCollapse: s.toggleCollapse, + // ... other actions + })), +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/components/outline/node-block.tsx` around lines 16 - 33, Multiple separate useOutlineStore selectors (useOutlineStore used to get node, activeNodeId, selectedNodeId, cursorPosition, activateNode, toggleCollapse, updateNodeContent, createNodeAfter, deleteNode, indentNode, outdentNode, moveNodeUp, moveNodeDown, getPreviousVisibleNode, getNextVisibleNode, nodes) cause many subscriptions and extra re-renders; refactor to a single useOutlineStore call that returns an object containing all needed values and action functions (or use Zustand's shallow comparator) so the component subscribes once—replace the individual selectors with one selector that returns { node, activeNodeId, selectedNodeId, cursorPosition, activateNode, toggleCollapse, updateNodeContent, createNodeAfter, deleteNode, indentNode, outdentNode, moveNodeUp, moveNodeDown, getPreviousVisibleNode, getNextVisibleNode, nodes } and import/use shallow equality if you keep returning an object.apps/nxus-editor/src/components/outline/node-content.tsx (3)
57-60: Redundant focus call.
el.focus()is already called at line 44. The second call at line 59 is redundant since focus has already been set.♻️ Proposed simplification
} else if (!textNode && content === '') { - // Empty node — just focus - el.focus() + // Empty node — focus already set above }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/components/outline/node-content.tsx` around lines 57 - 60, The second el.focus() call in the outline NodeContent component is redundant because focus() is already invoked earlier; remove the duplicate call inside the else if (!textNode && content === '') branch in node-content.tsx (or consolidate focus logic so focus() is called only once, e.g., move focus handling into a single place in the component render/effect where el is available, referencing the same variable names el, textNode, and content to locate the code).
5-6: Remove unusednodeIdprop.
nodeIdis declared inNodeContentPropsbut never used in the component body. This appears to be dead code.♻️ Proposed fix
interface NodeContentProps { - nodeId: string content: string isActive: boolean🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/components/outline/node-content.tsx` around lines 5 - 6, The NodeContentProps interface declares a nodeId prop that is never used in the NodeContent component; remove the unused prop to eliminate dead code: update the NodeContentProps definition to drop nodeId (and any places that pass nodeId to NodeContent), or alternatively remove nodeId from the NodeContent function signature/props destructuring if present, ensuring any references to NodeContentProps, nodeId, and the NodeContent component are updated consistently.
134-147: Inactive state uses same ref as active state.Both the active contentEditable div (line 118) and inactive presentation div (line 136) share
editorRef. During state transitions, effects relying on this ref may observe stale elements. Consider using separate refs or adding a key to the inactive div for clearer reconciliation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/components/outline/node-content.tsx` around lines 134 - 147, The inactive presentation div and the active contentEditable both share editorRef, causing effects that read editorRef to see stale DOM during transitions; split the refs (e.g., keep editorRef for the editable and add viewRef for the non-editable presentation) or add a stable key to the inactive div so React remounts it, and then update any useEffect/useLayoutEffect that currently reads editorRef to conditionally read the correct ref (editorRef or viewRef) and clean up listeners accordingly; target the editorRef usage in this component (NodeContent / node-content.tsx) and adjust refs and effects to reference the new viewRef or keyed element.apps/nxus-editor/src/hooks/use-outline-sync.ts (1)
20-21: Store instance captured once may miss action reference stability.Calling
useOutlineStore()without a selector returns the entire store object. While the actions are stable, accessingstore.nodesinside callbacks captures the value at callback creation time rather than the current state. The code correctly usesuseOutlineStore.getState()for async reconciliation (line 60), but the pattern is inconsistent.♻️ Consider using getState() consistently for async operations
export function useOutlineSync() { - const store = useOutlineStore() + const store = useOutlineStore const contentTimers = useRef(new Map<string, NodeJS.Timeout>()) // Then in callbacks: - const newId = store.createNodeAfter(afterId) + const newId = store.getState().createNodeAfter(afterId)Alternatively, select only the actions needed since they're stable:
const { createNodeAfter, updateNodeContent, ... } = useOutlineStore( useShallow((s) => ({ createNodeAfter: s.createNodeAfter, ... })) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/hooks/use-outline-sync.ts` around lines 20 - 21, The hook captures the entire store via useOutlineStore() which causes callbacks to close over stale store.nodes; fix by either (A) consistently using useOutlineStore.getState() inside async callbacks (e.g., where reconciliation currently uses getState()) instead of referencing store.nodes, or (B) change the hook to select only stable action functions (e.g., createNodeAfter, updateNodeContent, deleteNode, etc.) via useOutlineStore(selector) (optionally with useShallow) so callbacks reference stable action refs and not the full store; update uses of store.nodes in callbacks to call getState().nodes when up-to-date state is required and remove direct closed-over store usage (also apply same change for contentTimers usage if it reads store state).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/nxus-editor/src/components/outline/outline-editor.tsx`:
- Around line 119-122: The outer wrapper's onClick (handleBackgroundClick) is
clearing active state for all bubbled clicks (including from Breadcrumbs and
NodeBlock); modify handleBackgroundClick (or the outer div's handler) to only
clear when the click originated on the wrapper itself by checking e.target ===
e.currentTarget before calling deactivateNode(), or move the click handler to a
dedicated empty-space container instead; update references to
handleBackgroundClick and deactivateNode so child interactions no longer
inadvertently deactivate the node.
- Around line 73-76: The switch branch handling the 'Tab' key in
outline-editor.tsx currently calls e.preventDefault() (case 'Tab') which blocks
native focus traversal; change it to only preventDefault when the editor
implements tab-based indent/outdent (e.g., when functions like
handleIndent/handleOutdent or a prop/flag such as supportsTabIndent are present
and will handle the key). If those handlers/flags are not available, remove or
skip e.preventDefault() so Tab falls through to the browser; if conditional,
check the specific symbol (supportsTabIndent or handleIndent/handleOutdent)
before calling e.preventDefault() and performing indent/outdent logic. Ensure
the change is made inside the same switch/case handling the 'Tab' key.
In `@apps/nxus-editor/src/hooks/use-outline-sync.ts`:
- Around line 57-88: The state reconciliation currently reads from
useOutlineStore.getState() and then calls useOutlineStore.setState(...), risking
lost concurrent updates; change this to the functional updater form
useOutlineStore.setState((state) => { ... }) so the mutation is applied
atomically against the latest state—inside the updater clone state.nodes into a
new Map, locate tempNode via newId, delete newId, insert result.nodeId with
updated id, update the parent node's children array replacing newId with
result.nodeId, and update activeNodeId/selectedNodeId conditionally based on
whether they equal newId; reference the symbols useOutlineStore, nodes, newId,
tempNode, result.nodeId, activeNodeId, and selectedNodeId when making the
change.
In `@apps/nxus-editor/src/routes/__root.tsx`:
- Around line 101-125: The ScrollbarManager's single shared timeout causes
data-scrolling to stick when multiple elements scroll; update the implementation
to track timers per element (e.g., use a WeakMap<HTMLElement|Document,
NodeJS.Timeout> keyed by the element/document) inside ScrollbarManager, then in
handleScroll use the event target element (or document.documentElement) to clear
and set only that element's timeout, store the new timeout in the map, and when
the timeout fires removeAttribute('data-scrolling') and delete the map entry;
also update the cleanup to iterate the map and clear all timeouts and remove the
scroll listener; reference ScrollbarManager, handleScroll, timeout,
element.setAttribute, element.removeAttribute and the map you add.
- Around line 163-167: The root component currently passes a shared
"queryClient" into QueryClientProvider, which can leak cache across SSR
requests; instead, create a new QueryClient per server render and only reuse a
singleton in the browser. Fix by initializing the client inside the component
(e.g., a local queryClient) using a factory (new QueryClient()) on the server,
and on the client check typeof window !== 'undefined' (or globalThis) to
store/retrieve a singleton (e.g., globalThis.__REACT_QUERY_CLIENT__) so
QueryClientProvider receives a per-request instance on SSR but a reused instance
in the browser; update references to the exported/shared queryClient variable
and ensure QueryClientProvider is passed this local queryClient.
In `@apps/nxus-editor/src/services/outline.server.ts`:
- Around line 44-59: The computed colorValue is never used when building
outlineNode.supertags — replace the hardcoded color: null in the supertags
mapping with the appropriate color: use colorValue (from const colorValue =
getProperty<string>(assembled, FIELD_NAMES.COLOR) ...) if the color is shared
across the assembled node, or fetch a per-supertag color from each supertag
(e.g., st.color or getProperty on st) if each supertag has its own color; update
the mapping inside outlineNode (the assembled.supertags.map callback) to assign
the correct color field instead of null.
In `@apps/nxus-editor/src/stores/outline.store.ts`:
- Around line 38-44: The midpoint integer logic in generateOrderBetween causes
collisions for consecutive integer keys; replace it to use the
fractional-indexing library so keys are lexicographically sortable and support
infinite insertions. Modify generateOrderBetween to import and call the
fractional-indexing generator (e.g., generateBetween or equivalent export)
instead of computing Math.floor midpoints, passing null/undefined as lower/upper
bounds when appropriate, and then pass that fractional key into the existing
generateOrder wrapper if still needed; keep references to generateOrderBetween
and generateOrder so callers and any existing formatting remain unchanged.
In `@apps/nxus-editor/src/styles.css`:
- Around line 90-109: Move all theme `@import` statements so they appear
immediately after the Tailwind directives and before any other CSS rules;
specifically, relocate the block of `@import` lines currently placed after the
:root and .dark rules to a position just after the Tailwind directives at the
top of apps/nxus-editor/src/styles.css and before the :root and .dark rule
declarations so the imports comply with the CSS requirement that `@import` precede
other rules.
- Around line 225-229: The CSS rule for .editable[contenteditable='true'] uses
the deprecated word-break: break-word; update this selector to remove that
deprecated property and instead add modern overflow-wrap and explicit word-break
values (e.g., overflow-wrap: anywhere; and word-break: normal;) to restore
desired breaking behavior while satisfying stylelint and spec guidance.
In `@apps/nxus-gateway/src/config/mini-apps.ts`:
- Line 5: The MiniApp interface added a 'notepad' icon but iconMap in the UI
(symbol: iconMap in routes/index.tsx and similarly-named iconMap objects in
visual card components like glass-3d-cards.tsx) lacks a 'notepad' entry causing
iconMap['notepad'] to be undefined; fix by importing the notepad icon component
used elsewhere (add an import for the NotepadIcon or equivalent) and add a
'notepad' => NotepadIcon mapping to the iconMap in routes/index.tsx and
replicate the same mapping in every visual card component that defines its own
iconMap (search for iconMap definitions and add the 'notepad' key), ensuring no
runtime undefined lookups remain.
In `@docs/outline-editor-prd.md`:
- Around line 32-41: The fenced code blocks in the PRD (the example task block
starting with "- My Task `#task`" and the other examples referenced) are missing
language identifiers which triggers MD040; update each triple-backtick fence to
include a language token (e.g., ```text, ```typescript or ```tsx) so linting and
editor rendering work correctly for the blocks shown around the task example and
the other ranges called out (245-249, 313-345).
- Line 15: Choose one canonical sibling-order field name (e.g., use "order") and
update all occurrences of the alternative ("field:order") to that single name
across the document so the PRD, implementation plan, API examples, DB schema
references, and any sample payloads all match; specifically, replace every
"field:order" instance with "order" (while leaving other fields like "ownerId"
unchanged) and ensure the prose that defines sibling ordering consistently
references the chosen symbol.
- Line 7: The docs currently list scalar "plain data fields" (text, number,
boolean, date, select, url, email) alongside node reference fields (node, nodes)
but omit the Phase 2 first-class "json" field; update the opening architecture
paragraph and the value-kind / field-type contract to explicitly include "json"
as a scalar/primitive field type, and add a brief note where Phase 2 is
mentioned (e.g., in the "Phase 2" section and the value-kind table) that "json"
will have its own editor and schema requirements so earlier backend/schema specs
account for it (ensure references to "plain data fields", "node/nodeS", "Phase
2", and "json" are consistent).
- Line 150: The PR's doc claims natural-language date input ("tomorrow", "next
Friday") but the listed dependency date-fns cannot parse freeform phrases;
update the plan by either adding a named natural-language parser (e.g.,
chrono-node, timelang, any-date-parser) alongside date-fns for parsing and keep
date-fns for formatting, or change the feature scope to “date-picker-only” for
Phase 2; specifically update the row referencing "Formatted date" / "Date picker
popover" and the referenced dependency "date-fns" to reflect the chosen approach
and name the exact library/service that will handle natural-language parsing.
---
Nitpick comments:
In `@apps/nxus-editor/src/components/outline/bullet.tsx`:
- Around line 70-73: TreeLineProps declares an unused prop isLast; remove isLast
from the TreeLineProps interface and any related references so the TreeLine
component's props only include depth (update the TreeLineProps definition and
any destructuring or type annotations in the TreeLine component to match),
ensuring no other code expects isLast from TreeLine.
In `@apps/nxus-editor/src/components/outline/node-block.tsx`:
- Around line 179-183: sortedChildren is being recomputed every render; wrap the
sort logic in React's useMemo so it only recalculates when node.children or
nodes change. Replace the current const sortedChildren =
[...node.children].sort(...) with a useMemo that returns the sorted array and
lists node.children and nodes (or their relevant identity/stable references) as
dependencies; reference the sortedChildren variable, node.children, and nodes in
your change and ensure the comparator stays identical inside the memo.
- Around line 16-33: Multiple separate useOutlineStore selectors
(useOutlineStore used to get node, activeNodeId, selectedNodeId, cursorPosition,
activateNode, toggleCollapse, updateNodeContent, createNodeAfter, deleteNode,
indentNode, outdentNode, moveNodeUp, moveNodeDown, getPreviousVisibleNode,
getNextVisibleNode, nodes) cause many subscriptions and extra re-renders;
refactor to a single useOutlineStore call that returns an object containing all
needed values and action functions (or use Zustand's shallow comparator) so the
component subscribes once—replace the individual selectors with one selector
that returns { node, activeNodeId, selectedNodeId, cursorPosition, activateNode,
toggleCollapse, updateNodeContent, createNodeAfter, deleteNode, indentNode,
outdentNode, moveNodeUp, moveNodeDown, getPreviousVisibleNode,
getNextVisibleNode, nodes } and import/use shallow equality if you keep
returning an object.
In `@apps/nxus-editor/src/components/outline/node-content.tsx`:
- Around line 57-60: The second el.focus() call in the outline NodeContent
component is redundant because focus() is already invoked earlier; remove the
duplicate call inside the else if (!textNode && content === '') branch in
node-content.tsx (or consolidate focus logic so focus() is called only once,
e.g., move focus handling into a single place in the component render/effect
where el is available, referencing the same variable names el, textNode, and
content to locate the code).
- Around line 5-6: The NodeContentProps interface declares a nodeId prop that is
never used in the NodeContent component; remove the unused prop to eliminate
dead code: update the NodeContentProps definition to drop nodeId (and any places
that pass nodeId to NodeContent), or alternatively remove nodeId from the
NodeContent function signature/props destructuring if present, ensuring any
references to NodeContentProps, nodeId, and the NodeContent component are
updated consistently.
- Around line 134-147: The inactive presentation div and the active
contentEditable both share editorRef, causing effects that read editorRef to see
stale DOM during transitions; split the refs (e.g., keep editorRef for the
editable and add viewRef for the non-editable presentation) or add a stable key
to the inactive div so React remounts it, and then update any
useEffect/useLayoutEffect that currently reads editorRef to conditionally read
the correct ref (editorRef or viewRef) and clean up listeners accordingly;
target the editorRef usage in this component (NodeContent / node-content.tsx)
and adjust refs and effects to reference the new viewRef or keyed element.
In `@apps/nxus-editor/src/hooks/use-outline-sync.ts`:
- Around line 20-21: The hook captures the entire store via useOutlineStore()
which causes callbacks to close over stale store.nodes; fix by either (A)
consistently using useOutlineStore.getState() inside async callbacks (e.g.,
where reconciliation currently uses getState()) instead of referencing
store.nodes, or (B) change the hook to select only stable action functions
(e.g., createNodeAfter, updateNodeContent, deleteNode, etc.) via
useOutlineStore(selector) (optionally with useShallow) so callbacks reference
stable action refs and not the full store; update uses of store.nodes in
callbacks to call getState().nodes when up-to-date state is required and remove
direct closed-over store usage (also apply same change for contentTimers usage
if it reads store state).
In `@apps/nxus-editor/src/routes/__root.tsx`:
- Line 131: The RootDocument component uses an inline prop type; define a named
interface (e.g., interface RootDocumentProps { children: React.ReactNode }) and
replace the inline type in the RootDocument signature with that interface
(function RootDocument(props: RootDocumentProps) or destructure as function
RootDocument({ children }: RootDocumentProps)), ensuring the prop type follows
the repo convention and is easy to extend later; update any imports/exports or
usages if they rely on the previous inline signature.
- Around line 64-83: The applyTheme function currently trusts
localStorage.getItem('nxus-theme') and uses JSON.parse(stored).state directly,
which can lead to partial/malformed mutations of document.documentElement;
create a small Zod schema (e.g., { state: z.object({ colorMode:
z.enum(['light','dark']).default('dark'), palette:
z.string().refine(p=>ALL_PALETTES.includes(p) || p ===
'default').default('default') }).optional().default({ state: { colorMode:
'dark', palette: 'default' } }) }) and use schema.parse(parsedJson) before
touching classes, then compute colorMode/palette from the validated result and
only then remove/add classes on document.documentElement (keep using
ALL_PALETTES and preserve default fallbacks).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66a8b8d3-36bd-42c3-84b8-b9ac700f5dc7
⛔ Files ignored due to path filters (7)
editor-after-fix.pngis excluded by!**/*.pngeditor-after-indent.pngis excluded by!**/*.pngeditor-collapsed-multi.pngis excluded by!**/*.pngeditor-collapsed.pngis excluded by!**/*.pngeditor-dark-mode.pngis excluded by!**/*.pngeditor-initial.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
apps/nxus-editor/package.jsonapps/nxus-editor/src/components/outline/breadcrumbs.tsxapps/nxus-editor/src/components/outline/bullet.tsxapps/nxus-editor/src/components/outline/node-block.tsxapps/nxus-editor/src/components/outline/node-content.tsxapps/nxus-editor/src/components/outline/outline-editor.tsxapps/nxus-editor/src/hooks/use-outline-sync.tsapps/nxus-editor/src/lib/query-client.tsapps/nxus-editor/src/lib/seed-data.tsapps/nxus-editor/src/routeTree.gen.tsapps/nxus-editor/src/router.tsxapps/nxus-editor/src/routes/__root.tsxapps/nxus-editor/src/routes/index.tsxapps/nxus-editor/src/services/outline.server.tsapps/nxus-editor/src/stores/outline.store.tsapps/nxus-editor/src/styles.cssapps/nxus-editor/src/types/outline.tsapps/nxus-editor/tsconfig.jsonapps/nxus-editor/vite.config.tsapps/nxus-gateway/src/config/mini-apps.tsapps/nxus-gateway/vite.config.tsdocs/outline-editor-prd.mdpackage.json
| function ScrollbarManager() { | ||
| useEffect(() => { | ||
| let timeout: NodeJS.Timeout | ||
|
|
||
| const handleScroll = (e: Event) => { | ||
| const target = e.target | ||
| if (!target || !(target instanceof HTMLElement || target === document)) | ||
| return | ||
|
|
||
| const element = | ||
| target === document ? document.documentElement : (target as HTMLElement) | ||
|
|
||
| element.setAttribute('data-scrolling', 'true') | ||
|
|
||
| clearTimeout(timeout) | ||
| timeout = setTimeout(() => { | ||
| element.removeAttribute('data-scrolling') | ||
| }, 1000) | ||
| } | ||
|
|
||
| window.addEventListener('scroll', handleScroll, true) | ||
| return () => { | ||
| window.removeEventListener('scroll', handleScroll, true) | ||
| clearTimeout(timeout) | ||
| } |
There was a problem hiding this comment.
Track scroll timers per element.
timeout is shared across all scroll targets. If element A scrolls and element B scrolls before A's timeout fires, A's cleanup gets canceled and its data-scrolling attribute can stick forever.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/nxus-editor/src/routes/__root.tsx` around lines 101 - 125, The
ScrollbarManager's single shared timeout causes data-scrolling to stick when
multiple elements scroll; update the implementation to track timers per element
(e.g., use a WeakMap<HTMLElement|Document, NodeJS.Timeout> keyed by the
element/document) inside ScrollbarManager, then in handleScroll use the event
target element (or document.documentElement) to clear and set only that
element's timeout, store the new timeout in the map, and when the timeout fires
removeAttribute('data-scrolling') and delete the map entry; also update the
cleanup to iterate the map and clear all timeouts and remove the scroll
listener; reference ScrollbarManager, handleScroll, timeout,
element.setAttribute, element.removeAttribute and the map you add.
| <QueryClientProvider client={queryClient}> | ||
| <ThemeProvider /> | ||
| <ScrollbarManager /> | ||
| {children} | ||
| </QueryClientProvider> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the files mentioned in the review
fd -t f "__root.tsx"Repository: popemkt/nxus
Length of output: 293
🏁 Script executed:
# Find query-client.ts
fd -t f "query-client.ts"Repository: popemkt/nxus
Length of output: 305
🏁 Script executed:
# Check the project structure
ls -la apps/nxus-editor/ 2>/dev/null | head -20Repository: popemkt/nxus
Length of output: 409
🏁 Script executed:
# Read the __root.tsx file, focusing on the relevant lines
cat -n apps/nxus-editor/src/routes/__root.tsx | sed -n '155,175p'Repository: popemkt/nxus
Length of output: 532
🏁 Script executed:
# Read query-client.ts to see the singleton setup
cat -n apps/nxus-editor/src/lib/query-client.tsRepository: popemkt/nxus
Length of output: 294
🏁 Script executed:
# Check vite.config.ts for SSR settings
cat -n apps/nxus-editor/vite.config.tsRepository: popemkt/nxus
Length of output: 1366
🏁 Script executed:
# Check package.json for framework details
cat -n apps/nxus-editor/package.jsonRepository: popemkt/nxus
Length of output: 1577
🏁 Script executed:
# Check if __root.tsx has any loader or server-side specific code
cat -n apps/nxus-editor/src/routes/__root.tsx | head -40Repository: popemkt/nxus
Length of output: 1095
🏁 Script executed:
# Look for server entry points
fd -t f "entry" apps/nxus-editor/src/Repository: popemkt/nxus
Length of output: 38
🏁 Script executed:
# Search for server-side code or RootRoute setup
rg -A 5 "RootRoute\|createRootRoute" apps/nxus-editor/src/routes/__root.tsxRepository: popemkt/nxus
Length of output: 38
🏁 Script executed:
# Check if QueryClient is being created per-request anywhere
rg -r "new QueryClient" apps/nxus-editor/src/Repository: popemkt/nxus
Length of output: 817
🏁 Script executed:
# Look for any server-specific setup in the root or app files
fd -t f -e tsx -e ts "root\|app\|main\|index" apps/nxus-editor/src/ | head -10Repository: popemkt/nxus
Length of output: 38
🏁 Script executed:
# Look for server configuration or entry point in TanStack React Start
rg -l "server" apps/nxus-editor/src/ | grep -E "\.(ts|tsx)$"Repository: popemkt/nxus
Length of output: 150
🏁 Script executed:
# Check the actual exports and structure of __root.tsx
cat -n apps/nxus-editor/src/routes/__root.tsx | tail -20Repository: popemkt/nxus
Length of output: 704
🏁 Script executed:
# Look for any router or app configuration that might show how SSR works
fd -t f -e tsx "router\|app\|main" apps/nxus-editor/src/Repository: popemkt/nxus
Length of output: 38
🏁 Script executed:
# Check root route more completely
wc -l apps/nxus-editor/src/routes/__root.tsxRepository: popemkt/nxus
Length of output: 98
🏁 Script executed:
# Look at the complete __root.tsx component
cat -n apps/nxus-editor/src/routes/__root.tsxRepository: popemkt/nxus
Length of output: 5476
🏁 Script executed:
# Look for route loaders, beforeLoad, or server functions that might cache data
rg "loader\|beforeLoad\|serverFn" apps/nxus-editor/src/routes/Repository: popemkt/nxus
Length of output: 38
🏁 Script executed:
# Check if there's any per-request QueryClient reset or creation
rg "QueryClient\|queryClient" apps/nxus-editor/src/ -A 2 -B 2Repository: popemkt/nxus
Length of output: 38
🏁 Script executed:
# Look at other routes to see if there's a pattern for SSR
fd -t f "__root\|\.route\|router" apps/nxus-editor/src/routes/ | head -10Repository: popemkt/nxus
Length of output: 38
🏁 Script executed:
# Check TanStack React Start version and any SSR-specific patterns in docs or comments
rg -i "ssr\|server" apps/nxus-editor/ --type json --type ts --type tsx | head -20Repository: popemkt/nxus
Length of output: 83
🏁 Script executed:
# Search for serverFn or server-side functions more broadly
rg "serverFn\|server\(" apps/nxus-editor/src/ --type ts --type-list | head -20Repository: popemkt/nxus
Length of output: 445
🏁 Script executed:
# Look at services that might have server code
cat -n apps/nxus-editor/src/services/outline.server.ts | head -30Repository: popemkt/nxus
Length of output: 1096
🏁 Script executed:
# Search for any API or server routes
fd -t f -e ts -e tsx apps/nxus-editor/src/ | grep -E "(server|api)" | head -10Repository: popemkt/nxus
Length of output: 445
🏁 Script executed:
# Check nitro/server directory if exists
ls -la apps/nxus-editor/ | grep -E "server|api|nitro"Repository: popemkt/nxus
Length of output: 38
🏁 Script executed:
# Look for where queries might be pre-fetched
rg "prefetch\|dehydrate" apps/nxus-editor/src/Repository: popemkt/nxus
Length of output: 38
🏁 Script executed:
# Verify if there's any actual server-side data fetching into QueryClient by checking route handlers
rg "useQuery\|useMutation\|useInfiniteQuery" apps/nxus-editor/src/routes/Repository: popemkt/nxus
Length of output: 38
🏁 Script executed:
# Check if there are any route loaders that might populate the cache
rg "loader:" apps/nxus-editor/src/routes/Repository: popemkt/nxus
Length of output: 38
🏁 Script executed:
# Verify TanStack React Start version and whether it has built-in SSR QueryClient patterns
cat apps/nxus-editor/package.json | grep -A 1 "react-start"Repository: popemkt/nxus
Length of output: 139
🏁 Script executed:
# Look for any hydration or dehydration patterns specific to QueryClient
rg "Hydrate\|dehydrate\|getDehydratedState" apps/nxus-editor/src/Repository: popemkt/nxus
Length of output: 38
Create per-request QueryClient instances for SSR compatibility.
The root component renders on the server during each request. While no data is currently being prefetched into the QueryClient server-side, the pattern of reusing a singleton QueryClient across requests creates a vulnerability: if server-side data fetching is added via loaders or server functions in the future, that cache would be shared across requests and leak user data.
Create a new QueryClient per request/render on the server, and reuse a singleton only in the browser.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/nxus-editor/src/routes/__root.tsx` around lines 163 - 167, The root
component currently passes a shared "queryClient" into QueryClientProvider,
which can leak cache across SSR requests; instead, create a new QueryClient per
server render and only reuse a singleton in the browser. Fix by initializing the
client inside the component (e.g., a local queryClient) using a factory (new
QueryClient()) on the server, and on the client check typeof window !==
'undefined' (or globalThis) to store/retrieve a singleton (e.g.,
globalThis.__REACT_QUERY_CLIENT__) so QueryClientProvider receives a per-request
instance on SSR but a reused instance in the browser; update references to the
exported/shared queryClient variable and ensure QueryClientProvider is passed
this local queryClient.
|
|
||
| ### The Outline as Node Tree | ||
|
|
||
| Every bullet in the outline is a **node** in the database. Indentation represents the `ownerId` (parent) relationship. Sibling order is maintained via an `order` property. The outline is a recursive tree: each node can have children, which can have children, infinitely deep. |
There was a problem hiding this comment.
Use one canonical order field name.
This PRD defines sibling ordering as order early on, then switches to field:order in the implementation plan. That mismatch is enough to send the DB service and editor down different paths. Please pick one schema name and use it consistently throughout the document.
Also applies to: 125-128, 351-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/outline-editor-prd.md` at line 15, Choose one canonical sibling-order
field name (e.g., use "order") and update all occurrences of the alternative
("field:order") to that single name across the document so the PRD,
implementation plan, API examples, DB schema references, and any sample payloads
all match; specifically, replace every "field:order" instance with "order"
(while leaving other fields like "ownerId" unchanged) and ensure the prose that
defines sibling ordering consistently references the chosen symbol.
| ``` | ||
| - My Task #task | ||
| > Status: In Progress ← plain select field, inline dropdown | ||
| > Due Date: Mar 15, 2024 ← plain date field, date picker on click | ||
| > Priority: 3 ← plain number field, inline number input | ||
| > Assignee: [John Smith] ← node reference field, navigable chip | ||
| > Tags: [Frontend] [Urgent] ← multi-node reference, chips | ||
| - First subtask ← regular child node | ||
| - Second subtask ← regular child node | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to the fenced code blocks.
markdownlint is already flagging these fences with MD040. Adding a language like text, typescript, or tsx will clear lint and improve rendering in editors and GitHub.
Also applies to: 245-249, 313-345
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/outline-editor-prd.md` around lines 32 - 41, The fenced code blocks in
the PRD (the example task block starting with "- My Task `#task`" and the other
examples referenced) are missing language identifiers which triggers MD040;
update each triple-backtick fence to include a language token (e.g., ```text,
```typescript or ```tsx) so linting and editor rendering work correctly for the
blocks shown around the task example and the other ranges called out (245-249,
313-345).
Replace mock seed data with real database integration: - Fetch workspace roots and node trees from DB on mount - Optimistic Zustand mutations with fire-and-forget server persistence - Debounced content saves, temp-to-real ID replacement on create - Deterministic hash-based supertag color fallback - Cmd+click bullet to zoom/focus into a node with breadcrumb navigation - Virtual workspace root to hold top-level nodes (ownerId=null) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| let timeout: NodeJS.Timeout | ||
|
|
||
| const handleScroll = (e: Event) => { | ||
| const target = e.target | ||
| if (!target || !(target instanceof HTMLElement || target === document)) | ||
| return | ||
|
|
||
| const element = | ||
| target === document ? document.documentElement : (target as HTMLElement) | ||
|
|
||
| element.setAttribute('data-scrolling', 'true') | ||
|
|
||
| clearTimeout(timeout) | ||
| timeout = setTimeout(() => { | ||
| element.removeAttribute('data-scrolling') | ||
| }, 1000) | ||
| } |
There was a problem hiding this comment.
🟡 ScrollbarManager uses single shared timeout, leaving stale data-scrolling on elements
In __root.tsx:101-128, ScrollbarManager captures scroll events from any element but uses a single timeout variable. When element A scrolls and then element B scrolls before the 1-second timeout, clearTimeout(timeout) cancels the pending cleanup for element A, and the new timeout only removes data-scrolling from element B. Element A's data-scrolling='true' attribute is never removed, leaving scrollbar thumb permanently visible on that element.
| let timeout: NodeJS.Timeout | |
| const handleScroll = (e: Event) => { | |
| const target = e.target | |
| if (!target || !(target instanceof HTMLElement || target === document)) | |
| return | |
| const element = | |
| target === document ? document.documentElement : (target as HTMLElement) | |
| element.setAttribute('data-scrolling', 'true') | |
| clearTimeout(timeout) | |
| timeout = setTimeout(() => { | |
| element.removeAttribute('data-scrolling') | |
| }, 1000) | |
| } | |
| const timeouts = new Map<EventTarget, NodeJS.Timeout>() | |
| const handleScroll = (e: Event) => { | |
| const target = e.target | |
| if (!target || !(target instanceof HTMLElement || target === document)) | |
| return | |
| const element = | |
| target === document ? document.documentElement : (target as HTMLElement) | |
| element.setAttribute('data-scrolling', 'true') | |
| const existing = timeouts.get(target) | |
| if (existing) clearTimeout(existing) | |
| timeouts.set(target, setTimeout(() => { | |
| element.removeAttribute('data-scrolling') | |
| timeouts.delete(target) | |
| }, 1000)) | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
apps/nxus-editor/src/components/outline/outline-editor.tsx (2)
231-234:⚠️ Potential issue | 🟠 MajorThe wrapper still treats bubbled clicks as background clicks.
The
outline-bodyguard helps, but the outeronClickstill sees descendant clicks. Any child that doesn't stop propagation can immediately deactivate the node again.Minimal fix
- <div - className="outline-editor flex h-full flex-col" - onClick={handleBackgroundClick} - > + <div className="outline-editor flex h-full flex-col">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/components/outline/outline-editor.tsx` around lines 231 - 234, The wrapper's onClick handler (attached to the "outline-editor" div) still responds to bubbled clicks from children; update the handleBackgroundClick implementation to only treat events as background clicks when the event target is the container itself (i.e., check event.target === event.currentTarget) so descendant clicks won't deactivate the node; ensure the handler's event parameter type (e.g., React.MouseEvent) is correct and keep the existing "outline-body" guard intact.
161-164:⚠️ Potential issue | 🟠 MajorLet
Tabfall through until this view actually handles it.This still blocks native focus movement without providing an editor action, so keyboard users can get stuck on the outline when a node is only selected.
Minimal fix
case 'Tab': { - e.preventDefault() break }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/components/outline/outline-editor.tsx` around lines 161 - 164, The keydown handler in the OutlineEditor is intercepting the 'Tab' key and calling e.preventDefault(), which blocks native focus movement; remove the explicit handling for 'Tab' in the switch (i.e., delete the case 'Tab' block or let it fall through by not calling e.preventDefault() / break) so the event is not prevented and the browser can manage focus until the view implements Tab behavior. Target the switch handling 'Tab' inside the outline-editor keydown handler (the case 'Tab' in outline-editor.tsx) and ensure you do not call preventDefault() or stopPropagation for that key.apps/nxus-editor/src/stores/outline.store.ts (1)
39-45:⚠️ Potential issue | 🟠 MajorMidpoint ordering still runs out of space.
Once two sibling orders become adjacent,
generateOrderBetween(a, b)returnsaagain. That creates duplicate sort keys, and sibling order becomes unstable after enough insertions unless you renormalize or switch to a dense ordering scheme.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/stores/outline.store.ts` around lines 39 - 45, generateOrderBetween can return a duplicate when a and b are adjacent because mid rounds to a; detect when mid <= parseInt(a, 10) (or mid === parseInt(a, 10)) and handle that case instead of returning generateOrder(mid). Fix: in generateOrderBetween, after computing mid, if mid <= parseInt(a, 10) then trigger a rebalancing of sibling orders (implement a helper like rebalanceSiblingOrders or renormalizeOrders) which walks the affected siblings and reassigns new spaced orders via generateOrder (or fall back to a dense/fractional indexing strategy); otherwise return generateOrder(mid). Reference the function generateOrderBetween and the helper rebalanceSiblingOrders/renormalizeOrders (or replace with a fractional indexing library) so adjacent-order collisions are resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/nxus-editor/src/components/outline/node-block.tsx`:
- Around line 89-117: The Backspace handlers currently call deleteNode(nodeId)
which removes the node and its subtree; modify the logic in the empty-node
branch and the merge-at-zero branch to first check for children (e.g.,
node.children or scanning nodes for parentId === nodeId) and avoid destructive
deletion: if children exist, either reparent them to node.parentId (or to prevId
when merging) by updating their parent reference before deleting, or abort the
delete and place the caret appropriately; update the code paths around
deleteNode(nodeId), updateNodeContent(prevId,...), activateNode(prevId,...) and
any callers that assume subtree deletion so children are preserved/reparented
instead of dropped.
In `@apps/nxus-editor/src/services/outline.server.ts`:
- Around line 197-222: The reparentNodeServerFn currently writes ownerId
immediately via db.update(nodes)...where(eq(nodes.id, ctx.data.nodeId)) without
validating the newParentId; before updating, fetch the target parent and the
current node (using nodes and db), reject if ctx.data.newParentId ===
ctx.data.nodeId, reject if newParentId is not null but the parent row does not
exist, and reject if the target parent is a descendant of the node (to prevent
cycles) by traversing parents/ancestors; perform these checks (and ideally wrap
the read+write in a transaction) and return a clear error instead of performing
the update when any validation fails.
- Around line 183-191: The server handler deleteNodeServerFn currently calls
deleteNode(db, ctx.data.nodeId) which only soft-deletes the single parent row;
update the flow to either cascade-soft-delete all descendants or reparent them
before returning success. Modify or replace the call to deleteNode in
deleteNodeServerFn to invoke a new or existing service that performs a
recursive/cascade soft-delete (e.g., cascadeDeleteNode(db, nodeId) or
deleteNodeAndDescendants) or run a transaction that marks all descendant rows as
deleted (or reassigns their parent) via the node service in libs/nxus-db
(node.service.ts); ensure you still use initDatabaseWithBootstrap to get db and
return success only after the cascade/reparent operation completes.
- Around line 24-25: The current depth cap (const maxDepth = ctx.data.depth ??
10) silently truncates descendant trees for OutlineEditor; modify
outline.server.ts to detect when the returned subtree was cut off and include an
explicit truncation signal instead of hiding nodes. Specifically, keep using
ctx.data.depth but when the data source returns a node set equal to the cap (or
when you detect there are more children beyond maxDepth), add a boolean flag
like truncated: true on that node (and/or include a nextDepth token) so the
client (OutlineEditor) can lazy-load deeper levels; ensure the code paths in the
request handler that use maxDepth and ctx.data.depth set this flag whenever
descendants were limited.
In `@apps/nxus-editor/src/stores/outline.store.ts`:
- Around line 74-75: Update the setRootNodeId setter so changing the root also
clears any off-screen selection: when implementing setRootNodeId (currently
"setRootNodeId: (id) => set({ rootNodeId: id })"), include resetting
selectedNodeId and activeNodeId (e.g., set them to null/undefined) in the same
set call to prevent hotkeys from acting on nodes outside the new root.
---
Duplicate comments:
In `@apps/nxus-editor/src/components/outline/outline-editor.tsx`:
- Around line 231-234: The wrapper's onClick handler (attached to the
"outline-editor" div) still responds to bubbled clicks from children; update the
handleBackgroundClick implementation to only treat events as background clicks
when the event target is the container itself (i.e., check event.target ===
event.currentTarget) so descendant clicks won't deactivate the node; ensure the
handler's event parameter type (e.g., React.MouseEvent) is correct and keep the
existing "outline-body" guard intact.
- Around line 161-164: The keydown handler in the OutlineEditor is intercepting
the 'Tab' key and calling e.preventDefault(), which blocks native focus
movement; remove the explicit handling for 'Tab' in the switch (i.e., delete the
case 'Tab' block or let it fall through by not calling e.preventDefault() /
break) so the event is not prevented and the browser can manage focus until the
view implements Tab behavior. Target the switch handling 'Tab' inside the
outline-editor keydown handler (the case 'Tab' in outline-editor.tsx) and ensure
you do not call preventDefault() or stopPropagation for that key.
In `@apps/nxus-editor/src/stores/outline.store.ts`:
- Around line 39-45: generateOrderBetween can return a duplicate when a and b
are adjacent because mid rounds to a; detect when mid <= parseInt(a, 10) (or mid
=== parseInt(a, 10)) and handle that case instead of returning
generateOrder(mid). Fix: in generateOrderBetween, after computing mid, if mid <=
parseInt(a, 10) then trigger a rebalancing of sibling orders (implement a helper
like rebalanceSiblingOrders or renormalizeOrders) which walks the affected
siblings and reassigns new spaced orders via generateOrder (or fall back to a
dense/fractional indexing strategy); otherwise return generateOrder(mid).
Reference the function generateOrderBetween and the helper
rebalanceSiblingOrders/renormalizeOrders (or replace with a fractional indexing
library) so adjacent-order collisions are resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c3ec2d5-f43c-4801-ab8f-7ef8b724a311
📒 Files selected for processing (9)
apps/nxus-editor/src/components/outline/breadcrumbs.tsxapps/nxus-editor/src/components/outline/bullet.tsxapps/nxus-editor/src/components/outline/node-block.tsxapps/nxus-editor/src/components/outline/outline-editor.tsxapps/nxus-editor/src/hooks/use-outline-sync.tsapps/nxus-editor/src/lib/supertag-colors.tsapps/nxus-editor/src/services/outline.server.tsapps/nxus-editor/src/stores/outline.store.tsapps/nxus-editor/src/types/outline.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/nxus-editor/src/components/outline/bullet.tsx
- apps/nxus-editor/src/hooks/use-outline-sync.ts
- apps/nxus-editor/src/types/outline.ts
| const maxDepth = ctx.data.depth ?? 10 | ||
|
|
There was a problem hiding this comment.
The default depth cap returns partial trees as if they were complete.
OutlineEditor calls this endpoint without overriding depth, so descendants beyond level 10 are silently omitted and the client has no way to recover them. Either fetch the full subtree here or return an explicit truncation signal that the client can lazy-load.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/nxus-editor/src/services/outline.server.ts` around lines 24 - 25, The
current depth cap (const maxDepth = ctx.data.depth ?? 10) silently truncates
descendant trees for OutlineEditor; modify outline.server.ts to detect when the
returned subtree was cut off and include an explicit truncation signal instead
of hiding nodes. Specifically, keep using ctx.data.depth but when the data
source returns a node set equal to the cap (or when you detect there are more
children beyond maxDepth), add a boolean flag like truncated: true on that node
(and/or include a nextDepth token) so the client (OutlineEditor) can lazy-load
deeper levels; ensure the code paths in the request handler that use maxDepth
and ctx.data.depth set this flag whenever descendants were limited.
| export const deleteNodeServerFn = createServerFn({ method: 'POST' }) | ||
| .inputValidator(z.object({ nodeId: z.string() })) | ||
| .handler(async (ctx) => { | ||
| const { initDatabaseWithBootstrap, deleteNode } = await import( | ||
| '@nxus/db/server' | ||
| ) | ||
| const db = await initDatabaseWithBootstrap() | ||
| deleteNode(db, ctx.data.nodeId) | ||
| return { success: true as const } |
There was a problem hiding this comment.
This delete path only soft-deletes the parent row.
libs/nxus-db/src/services/node.service.ts only marks ctx.data.nodeId deleted. The client store removes whole subtrees, so a refresh can leave descendants orphaned and even reopen the workspace on an arbitrary leftover node. Cascade the soft delete or intentionally reparent descendants before returning success.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/nxus-editor/src/services/outline.server.ts` around lines 183 - 191, The
server handler deleteNodeServerFn currently calls deleteNode(db,
ctx.data.nodeId) which only soft-deletes the single parent row; update the flow
to either cascade-soft-delete all descendants or reparent them before returning
success. Modify or replace the call to deleteNode in deleteNodeServerFn to
invoke a new or existing service that performs a recursive/cascade soft-delete
(e.g., cascadeDeleteNode(db, nodeId) or deleteNodeAndDescendants) or run a
transaction that marks all descendant rows as deleted (or reassigns their
parent) via the node service in libs/nxus-db (node.service.ts); ensure you still
use initDatabaseWithBootstrap to get db and return success only after the
cascade/reparent operation completes.
| export const reparentNodeServerFn = createServerFn({ method: 'POST' }) | ||
| .inputValidator( | ||
| z.object({ | ||
| nodeId: z.string(), | ||
| newParentId: z.string().nullable(), | ||
| order: z.number().optional(), | ||
| }), | ||
| ) | ||
| .handler(async (ctx) => { | ||
| const { | ||
| initDatabaseWithBootstrap, | ||
| nodes, | ||
| eq, | ||
| setProperty, | ||
| SYSTEM_FIELDS, | ||
| } = await import('@nxus/db/server') | ||
| const db = await initDatabaseWithBootstrap() | ||
|
|
||
| // Update ownerId directly | ||
| db.update(nodes) | ||
| .set({ | ||
| ownerId: ctx.data.newParentId, | ||
| updatedAt: new Date(), | ||
| }) | ||
| .where(eq(nodes.id, ctx.data.nodeId)) | ||
| .run() |
There was a problem hiding this comment.
Reject cyclic or missing parents before writing ownerId.
This updates ownerId without checking newParentId === nodeId, descendant parents, or whether the target parent still exists. One bad request can persist a cycle and wedge the recursive loaders/renderers in this PR. As per coding guidelines, "Validate external inputs using Zod schemas before processing" and "Prioritize handling edge cases including errors, empty states, and loading states".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/nxus-editor/src/services/outline.server.ts` around lines 197 - 222, The
reparentNodeServerFn currently writes ownerId immediately via
db.update(nodes)...where(eq(nodes.id, ctx.data.nodeId)) without validating the
newParentId; before updating, fetch the target parent and the current node
(using nodes and db), reject if ctx.data.newParentId === ctx.data.nodeId, reject
if newParentId is not null but the parent row does not exist, and reject if the
target parent is a descendant of the node (to prevent cycles) by traversing
parents/ancestors; perform these checks (and ideally wrap the read+write in a
transaction) and return a clear error instead of performing the update when any
validation fails.
| setNodes: (nodes) => set({ nodes }), | ||
| setRootNodeId: (id) => set({ rootNodeId: id }), |
There was a problem hiding this comment.
Changing roots should clear hidden selection state.
setRootNodeId leaves selectedNodeId and activeNodeId untouched. After a breadcrumb/Cmd-click zoom, OutlineEditor hotkeys can still act on an off-screen node, so Enter/Delete/Space can mutate something the user can't see.
Minimal safe reset
- setRootNodeId: (id) => set({ rootNodeId: id }),
+ setRootNodeId: (id) =>
+ set({
+ rootNodeId: id,
+ activeNodeId: null,
+ selectedNodeId: null,
+ cursorPosition: 0,
+ }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/nxus-editor/src/stores/outline.store.ts` around lines 74 - 75, Update
the setRootNodeId setter so changing the root also clears any off-screen
selection: when implementing setRootNodeId (currently "setRootNodeId: (id) =>
set({ rootNodeId: id })"), include resetting selectedNodeId and activeNodeId
(e.g., set them to null/undefined) in the same set call to prevent hotkeys from
acting on nodes outside the new root.
- Fields: Extract node properties from DB and display as typed field rows (text, number, boolean, date, select, url, email, json) with inline click-to-edit controls and server persistence - Bullet icons: Diamond shape for supertag nodes, circle for plain nodes, with rounded-square halo on collapsed supertag nodes - PRD: Mark all completed/partial/pending features with checkboxes - Server: Add setFieldValueServerFn, field type resolution with cache, filter hidden system fields (order, supertag, extends, etc.) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (5)
docs/outline-editor-prd.md (1)
32-41:⚠️ Potential issue | 🟡 MinorAdd language identifiers to these fenced blocks.
They still trip MD040 and render worse in GitHub/editors without a language token such as
text,typescript, ortsx.Also applies to: 242-246, 310-342
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/outline-editor-prd.md` around lines 32 - 41, The fenced example blocks (e.g., the task example that begins with "- My Task `#task`" and other similar examples around the noted ranges) are missing language identifiers which triggers MD040 and causes poor rendering; update each triple-backtick fence to include an explicit language token such as "text", "typescript", or "tsx" (choose the most appropriate for the block content) so the blocks read like ```text or ```tsx instead of bare ``` to fix linting and GitHub/editor rendering for the examples referenced (including the shown task block and the other blocks around lines 242-246 and 310-342).apps/nxus-editor/src/components/outline/node-block.tsx (1)
90-118:⚠️ Potential issue | 🔴 CriticalBackspace here may still delete descendants, not just the current row.
Both branches call
deleteNode(nodeId)without handling children first. IfdeleteNoderemoves the subtree, backspacing an empty parent or merging at column 0 drops descendants as well.♻️ Minimal guard
if ( e.key === 'Backspace' && node?.content === '' && + node?.children.length === 0 && !e.metaKey && !e.ctrlKey ) { @@ - if (e.key === 'Backspace' && sel?.focusOffset === 0 && node?.content) { + if ( + e.key === 'Backspace' && + sel?.focusOffset === 0 && + node?.content && + node?.children.length === 0 + ) {Verify
deleteNodesemantics before landing this. If it deletes the subtree, these guards are still needed:#!/bin/bash fd 'use-outline-sync\.ts$' apps/nxus-editor/src -x sed -n '1,260p' {} echo '--- deleteNode call sites ---' rg -n -C3 '\bdeleteNode\b' apps/nxus-editor/src🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/components/outline/node-block.tsx` around lines 90 - 118, The Backspace handlers call deleteNode(nodeId) which may remove the entire subtree and drop descendants; before deleting or merging, check node.children (or use API like getChildren(nodeId)) and handle them: either prevent deletion when descendants exist, reparent/move children into prevId, or sequentially relocate children to prevId before calling deleteNode; update the two branches that call deleteNode (the empty-node branch and the merge branch in node-block.tsx) to detect descendants and perform the chosen preservation strategy, using getPreviousVisibleNode, nodes.get(prevId), updateNodeContent, activateNode and deleteNode as needed so descendants are not lost.apps/nxus-editor/src/components/outline/outline-editor.tsx (1)
163-165:⚠️ Potential issue | 🟠 MajorLet
Tabfall through until selection-mode indent/outdent exists.Right now this blocks native focus traversal and doesn't perform any outline action.
♻️ Minimal fix
case 'Tab': { - e.preventDefault() break }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/components/outline/outline-editor.tsx` around lines 163 - 165, Remove the unconditional Tab block that calls e.preventDefault() in the keydown switch inside the OutlineEditor component (the "case 'Tab': { e.preventDefault(); break }" branch) so Tab falls through to native focus traversal until you implement selection-mode indent/outdent; alternatively, replace it with a conditional that only prevents default and runs indent/outdent when a selection-based indent/outdent handler exists, otherwise do nothing so the browser handles focus traversal.apps/nxus-editor/src/services/outline.server.ts (2)
225-233:⚠️ Potential issue | 🟠 MajorDeleting a parent still leaves its descendants behind.
Line 232 only calls
deleteNode(), andlibs/nxus-db/src/services/node.service.ts:548-564soft-deletes a single row. The client removes whole subtrees, so a refresh can resurrect orphaned children under a deleted parent. Please cascade the soft delete or intentionally reparent descendants before returning success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/services/outline.server.ts` around lines 225 - 233, deleteNodeServerFn currently calls deleteNode(db, ctx.data.nodeId) which soft-deletes only the single row; you must cascade the soft-delete or reparent descendants before returning success. Modify the handler in deleteNodeServerFn to, after initializing the DB with initDatabaseWithBootstrap and before returning, either call a provided cascading function (e.g., deleteDescendants(db, ctx.data.nodeId) or softDeleteSubtree) or run a traversal/update to mark all children as deleted (or reparent them to a safe parent) using the node service APIs so no orphaned children remain; ensure you reference and call the new helper (or implement the traversal) alongside deleteNode so the entire subtree is handled atomically.
239-267:⚠️ Potential issue | 🔴 CriticalValidate the target parent and make reparenting atomic.
Line 257 updates
ownerIdbefore checkingnewParentId. That still allows self-parenting, descendant-parenting cycles, or moves under a missing/deleted node, and if Line 266 fails you keep a partially applied move. Please reject invalid parents first and wrap the owner/order writes in one transaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/services/outline.server.ts` around lines 239 - 267, Reject invalid newParentId before making changes and perform the ownerId/order updates atomically: when handling reparentNodeServerFn, if ctx.data.newParentId is not null first confirm a node with that id exists (using nodes) and that it is not the same as ctx.data.nodeId and not a descendant of ctx.data.nodeId (walk the ownerId chain via the nodes table to detect cycles). If validation fails, throw/return an error. Then perform the db.update(nodes).set({ ownerId, updatedAt }) and the optional setProperty(...) inside a single database transaction obtained from initDatabaseWithBootstrap/db.transaction so that both writes commit or roll back together. Ensure you reference nodes, setProperty, and SYSTEM_FIELDS.ORDER in the transaction block and abort the transaction on any failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/nxus-editor/src/components/outline/field-value.tsx`:
- Around line 367-376: The EmailField branch that renders when `value` is
present currently only returns a mailto anchor and never exposes a way back into
edit mode; update the `EmailField` rendering (the branch that checks `if
(value)`) to include the same secondary edit affordance used by `UrlField`
(e.g., a small edit button/icon that onClick sets `editing` to true and stops
propagation) so users can toggle back into the `editing` state; ensure the
mailto anchor still works, the edit button is accessible and visually consistent
with `UrlField`, and use the existing `editing` state handler in this component
to perform the toggle.
- Around line 111-115: NumberField: fix the commit callback (the function
referenced as commit) to treat an empty draft as a clear action instead of
coercing it to 0—if draft is '' call the clear path (e.g., invoke onChange with
the component's "empty" value) and only parse Number(draft) when draft is
non-empty; DateField: change the date parsing logic so date-only strings
("YYYY-MM-DD") are parsed locally by splitting the string and constructing a
local Date via new Date(year, monthIndex, day) (avoid using new Date(dateString)
which treats date-only strings as UTC); EmailField: update the non-edit
rendering to include an edit button like UrlField does (copy the edit
button/handler pattern used around the UrlField render code) that sets editing
mode so users can re-enter the email input. Ensure you reference and update the
relevant handlers (commit/setEditing/onChange) in each component.
In `@apps/nxus-editor/src/components/outline/fields-section.tsx`:
- Around line 46-58: The handler handleChange currently no-ops when
field.fieldSystemId is null (leaving the UI editable) and when
setFieldValueServerFn resolves it only returns { success: true } so the local
field.values stay stale; fix by disabling the input when field.fieldSystemId is
falsy (so unsaveable fields are not editable) and, in the success path of
setFieldValueServerFn in handleChange, dispatch or call the appropriate
outline.store update action (or update the relevant query cache) to apply the
new value to the store so the editor re-renders with the updated field.values;
also surface server errors via the existing catch path. Ensure you reference
handleChange, field.fieldSystemId, setFieldValueServerFn, the outline.server.ts
mutation response, and the outline.store.ts update/action when wiring this
change.
In `@apps/nxus-editor/src/components/outline/outline-editor.tsx`:
- Around line 43-46: The current check collapses a failed fetch
(rootResult.success === false) into the empty-state path; split the conditions
so failures are handled explicitly: if rootResult.success is false, call
setLoading(false) and surface the error (e.g., setError(rootResult.error ||
'Failed to fetch workspace roots') or throw) so the UI/logic can show a failure
state or retry, and only treat empty rootIds as the "no nodes found" path (i.e.,
use separate branches for rootResult.success and rootResult.rootIds.length ===
0). Ensure you reference rootResult.success, rootResult.rootIds, setLoading and
the component's error-handling state or handler when making the change.
In `@apps/nxus-editor/src/services/outline.server.ts`:
- Around line 180-199: The handler is passing parentId straight into createNode
which can create orphaned children; before calling createNode (after awaiting
initDatabaseWithBootstrap()), start a DB transaction, load the parent node
(e.g., use the node lookup function in node.service or a raw query against the
nodes table) and verify it exists and is not deleted, and only then pass ownerId
to createNode; if the parent is missing or deleted, abort the transaction and
throw a validation error. Ensure the parent existence check and createNode call
run in the same transaction (use the DB transaction API returned by
initDatabaseWithBootstrap), update the createNode call site to conditionally
include ownerId only when the validated parent is present, and return a clear
error when validation fails.
- Around line 289-306: The handler in outline.server.ts currently forwards
arbitrary ctx.data.value to setProperty without type-checking; update the
handler (the inputValidator/handler block that calls initDatabaseWithBootstrap
and setProperty) to first resolve the field schema for the given nodeId and
fieldSystemId (use the DB service helpers or node metadata APIs to fetch the
field definition), build/obtain a Zod (or existing) validator for that field
type, validate/coerce ctx.data.value against that schema, and only call
setProperty if validation passes; return a clear validation error response if
the value fails type/schema checks instead of persisting it. Ensure references
include initDatabaseWithBootstrap, setProperty, and the FieldSystemId usage so
reviewers can find the change.
- Around line 135-138: After calling loadNode(ctx.data.nodeId, 0) ensure you
explicitly detect a stale/missing root by checking nodeMap.has(ctx.data.nodeId);
if the root id is not present, return a failure result (e.g., { success: false,
error: 'root_not_found' } or similar) instead of returning success with an empty
nodes array. Update the return path that currently builds nodesArray and returns
{ success: true, nodes: nodesArray, rootId: ctx.data.nodeId } so it only
executes when nodeMap contains ctx.data.nodeId; otherwise return the not-found
failure immediately.
---
Duplicate comments:
In `@apps/nxus-editor/src/components/outline/node-block.tsx`:
- Around line 90-118: The Backspace handlers call deleteNode(nodeId) which may
remove the entire subtree and drop descendants; before deleting or merging,
check node.children (or use API like getChildren(nodeId)) and handle them:
either prevent deletion when descendants exist, reparent/move children into
prevId, or sequentially relocate children to prevId before calling deleteNode;
update the two branches that call deleteNode (the empty-node branch and the
merge branch in node-block.tsx) to detect descendants and perform the chosen
preservation strategy, using getPreviousVisibleNode, nodes.get(prevId),
updateNodeContent, activateNode and deleteNode as needed so descendants are not
lost.
In `@apps/nxus-editor/src/components/outline/outline-editor.tsx`:
- Around line 163-165: Remove the unconditional Tab block that calls
e.preventDefault() in the keydown switch inside the OutlineEditor component (the
"case 'Tab': { e.preventDefault(); break }" branch) so Tab falls through to
native focus traversal until you implement selection-mode indent/outdent;
alternatively, replace it with a conditional that only prevents default and runs
indent/outdent when a selection-based indent/outdent handler exists, otherwise
do nothing so the browser handles focus traversal.
In `@apps/nxus-editor/src/services/outline.server.ts`:
- Around line 225-233: deleteNodeServerFn currently calls deleteNode(db,
ctx.data.nodeId) which soft-deletes only the single row; you must cascade the
soft-delete or reparent descendants before returning success. Modify the handler
in deleteNodeServerFn to, after initializing the DB with
initDatabaseWithBootstrap and before returning, either call a provided cascading
function (e.g., deleteDescendants(db, ctx.data.nodeId) or softDeleteSubtree) or
run a traversal/update to mark all children as deleted (or reparent them to a
safe parent) using the node service APIs so no orphaned children remain; ensure
you reference and call the new helper (or implement the traversal) alongside
deleteNode so the entire subtree is handled atomically.
- Around line 239-267: Reject invalid newParentId before making changes and
perform the ownerId/order updates atomically: when handling
reparentNodeServerFn, if ctx.data.newParentId is not null first confirm a node
with that id exists (using nodes) and that it is not the same as ctx.data.nodeId
and not a descendant of ctx.data.nodeId (walk the ownerId chain via the nodes
table to detect cycles). If validation fails, throw/return an error. Then
perform the db.update(nodes).set({ ownerId, updatedAt }) and the optional
setProperty(...) inside a single database transaction obtained from
initDatabaseWithBootstrap/db.transaction so that both writes commit or roll back
together. Ensure you reference nodes, setProperty, and SYSTEM_FIELDS.ORDER in
the transaction block and abort the transaction on any failure.
In `@docs/outline-editor-prd.md`:
- Around line 32-41: The fenced example blocks (e.g., the task example that
begins with "- My Task `#task`" and other similar examples around the noted
ranges) are missing language identifiers which triggers MD040 and causes poor
rendering; update each triple-backtick fence to include an explicit language
token such as "text", "typescript", or "tsx" (choose the most appropriate for
the block content) so the blocks read like ```text or ```tsx instead of bare ```
to fix linting and GitHub/editor rendering for the examples referenced
(including the shown task block and the other blocks around lines 242-246 and
310-342).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78ee68e5-2cfe-48e7-a208-776a5a7fb194
📒 Files selected for processing (10)
apps/nxus-editor/src/components/outline/bullet.tsxapps/nxus-editor/src/components/outline/field-value.tsxapps/nxus-editor/src/components/outline/fields-section.tsxapps/nxus-editor/src/components/outline/node-block.tsxapps/nxus-editor/src/components/outline/outline-editor.tsxapps/nxus-editor/src/lib/seed-data.tsapps/nxus-editor/src/services/outline.server.tsapps/nxus-editor/src/stores/outline.store.tsapps/nxus-editor/src/types/outline.tsdocs/outline-editor-prd.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/nxus-editor/src/stores/outline.store.ts
| if (!rootResult.success || rootResult.rootIds.length === 0) { | ||
| setLoading(false) | ||
| return | ||
| } |
There was a problem hiding this comment.
Don't collapse success: false into the empty-state path.
A failed workspace-root fetch currently stops loading and later falls through to "No nodes found." That hides the load failure and makes recovery/debugging harder.
♻️ Minimal fix
- if (!rootResult.success || rootResult.rootIds.length === 0) {
+ if (!rootResult.success) {
+ setError('Failed to load workspace roots')
+ setLoading(false)
+ return
+ }
+
+ if (rootResult.rootIds.length === 0) {
setLoading(false)
return
}📝 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.
| if (!rootResult.success || rootResult.rootIds.length === 0) { | |
| setLoading(false) | |
| return | |
| } | |
| if (!rootResult.success) { | |
| setError('Failed to load workspace roots') | |
| setLoading(false) | |
| return | |
| } | |
| if (rootResult.rootIds.length === 0) { | |
| setLoading(false) | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/nxus-editor/src/components/outline/outline-editor.tsx` around lines 43 -
46, The current check collapses a failed fetch (rootResult.success === false)
into the empty-state path; split the conditions so failures are handled
explicitly: if rootResult.success is false, call setLoading(false) and surface
the error (e.g., setError(rootResult.error || 'Failed to fetch workspace roots')
or throw) so the UI/logic can show a failure state or retry, and only treat
empty rootIds as the "no nodes found" path (i.e., use separate branches for
rootResult.success and rootResult.rootIds.length === 0). Ensure you reference
rootResult.success, rootResult.rootIds, setLoading and the component's
error-handling state or handler when making the change.
| loadNode(ctx.data.nodeId, 0) | ||
|
|
||
| const nodesArray = Array.from(nodeMap.values()) | ||
| return { success: true as const, nodes: nodesArray, rootId: ctx.data.nodeId } |
There was a problem hiding this comment.
Return a not-found error for stale root IDs.
If loadNode() skips the requested node because it is missing or soft-deleted, Line 138 still returns { success: true, nodes: [], rootId }. That makes an invalid root indistinguishable from a valid empty tree and pushes the failure into the UI. Please fail this request explicitly once nodeMap does not contain ctx.data.nodeId.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/nxus-editor/src/services/outline.server.ts` around lines 135 - 138,
After calling loadNode(ctx.data.nodeId, 0) ensure you explicitly detect a
stale/missing root by checking nodeMap.has(ctx.data.nodeId); if the root id is
not present, return a failure result (e.g., { success: false, error:
'root_not_found' } or similar) instead of returning success with an empty nodes
array. Update the return path that currently builds nodesArray and returns {
success: true, nodes: nodesArray, rootId: ctx.data.nodeId } so it only executes
when nodeMap contains ctx.data.nodeId; otherwise return the not-found failure
immediately.
| .inputValidator( | ||
| z.object({ | ||
| content: z.string(), | ||
| parentId: z.string().nullable(), | ||
| order: z.number().optional(), | ||
| }), | ||
| ) | ||
| .handler(async (ctx) => { | ||
| const { | ||
| initDatabaseWithBootstrap, | ||
| createNode, | ||
| setProperty, | ||
| SYSTEM_FIELDS, | ||
| } = await import('@nxus/db/server') | ||
| const db = await initDatabaseWithBootstrap() | ||
|
|
||
| const nodeId = createNode(db, { | ||
| content: ctx.data.content, | ||
| ...(ctx.data.parentId ? { ownerId: ctx.data.parentId } : {}), | ||
| }) |
There was a problem hiding this comment.
Validate parentId before creating a child node.
Line 198 forwards parentId straight into createNode(), and libs/nxus-db/src/services/node.service.ts:463-511 persists ownerId as-is. A stale or deleted parent will therefore create an orphaned row that the workspace root query cannot recover. Please verify the parent exists and is not deleted before inserting, ideally in the same transaction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/nxus-editor/src/services/outline.server.ts` around lines 180 - 199, The
handler is passing parentId straight into createNode which can create orphaned
children; before calling createNode (after awaiting
initDatabaseWithBootstrap()), start a DB transaction, load the parent node
(e.g., use the node lookup function in node.service or a raw query against the
nodes table) and verify it exists and is not deleted, and only then pass ownerId
to createNode; if the parent is missing or deleted, abort the transaction and
throw a validation error. Ensure the parent existence check and createNode call
run in the same transaction (use the DB transaction API returned by
initDatabaseWithBootstrap), update the createNode call site to conditionally
include ownerId only when the validated parent is present, and return a clear
error when validation fails.
| .inputValidator( | ||
| z.object({ | ||
| nodeId: z.string(), | ||
| fieldSystemId: z.string(), | ||
| value: z.unknown(), | ||
| }), | ||
| ) | ||
| .handler(async (ctx) => { | ||
| const { initDatabaseWithBootstrap, setProperty } = await import( | ||
| '@nxus/db/server' | ||
| ) | ||
| const db = await initDatabaseWithBootstrap() | ||
| setProperty( | ||
| db, | ||
| ctx.data.nodeId, | ||
| ctx.data.fieldSystemId as import('@nxus/db/server').FieldSystemId, | ||
| ctx.data.value, | ||
| ) |
There was a problem hiding this comment.
Enforce field-type validation before writing arbitrary values.
Lines 293-305 accept any payload and pass it straight to setProperty(), and libs/nxus-db/src/services/node.service.ts:572-615 just JSON-serializes whatever it receives. That means a buggy client can persist strings into number fields, objects into dates, etc. Please resolve the field type server-side and validate ctx.data.value against the matching schema before writing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/nxus-editor/src/services/outline.server.ts` around lines 289 - 306, The
handler in outline.server.ts currently forwards arbitrary ctx.data.value to
setProperty without type-checking; update the handler (the
inputValidator/handler block that calls initDatabaseWithBootstrap and
setProperty) to first resolve the field schema for the given nodeId and
fieldSystemId (use the DB service helpers or node metadata APIs to fetch the
field definition), build/obtain a Zod (or existing) validator for that field
type, validate/coerce ctx.data.value against that schema, and only call
setProperty if validation passes; return a clear validation error response if
the value fails type/schema checks instead of persisting it. Ensure references
include initDatabaseWithBootstrap, setProperty, and the FieldSystemId usage so
reviewers can find the change.
…colors - Configure vitest for nxus-editor app - 29 tests for outline store: setNodes, activate/deactivate, select, toggleCollapse, updateNodeContent, createNodeAfter, deleteNode, indentNode, outdentNode, moveNodeUp/Down, getVisibleNodes, navigation - 5 tests for supertag color utility: determinism, uniqueness, edge cases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
16 e2e tests covering: - Page load (header, breadcrumbs, outline body) - Node rendering (blocks from DB, bullet icons) - Node activation & editing (click to edit, typing, click outside) - Keyboard operations (Enter creates sibling, Escape to selection mode, arrow nav) - Collapse/expand (bullet click toggle) - Zoom/focus (Cmd+click bullet, Home breadcrumb) - Supertag display (colored badges) - Fields display (field rows with indicators) Also register 'editor' app in e2e fixtures and navigation helpers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/nxus-editor/src/stores/outline.store.test.ts (1)
157-183: Consider adding a test foractiveNodeIdupdate after deletion.Based on the
deleteNodeimplementation (Context snippet 1), when the deleted node is the active node, the store updatesactiveNodeIdto the previous visible node. The current tests verify node removal and parent updates but don't explicitly test theactiveNodeIdtransition after deleting the active node.💡 Suggested additional test
it('updates activeNodeId to previous visible when deleting active node', () => { seedStore() useOutlineStore.getState().activateNode('b', 0) expect(useOutlineStore.getState().activeNodeId).toBe('b') useOutlineStore.getState().deleteNode('b') const state = useOutlineStore.getState() expect(state.nodes.has('b')).toBe(false) expect(state.activeNodeId).toBe('a2') // previous visible node })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/stores/outline.store.test.ts` around lines 157 - 183, Add a test that verifies activeNodeId is updated when the deleted node was active: after seedStore(), call activateNode('b', 0) and assert activeNodeId === 'b', then call deleteNode('b') and assert the node is removed (nodes.has('b') is false) and activeNodeId equals the previous visible node (e.g., 'a2'); place this alongside the other deleteNode tests so deleteNode, activateNode, activeNodeId, seedStore and WORKSPACE_ROOT_ID are exercised together.apps/nxus-editor/package.json (1)
27-27: Pinnitroto a concrete version or tested range.Using
latestmakes fresh installs and lockfile refreshes non-deterministic. The resolved version (3.0.1-alpha.2 from pnpm-lock.yaml) is an alpha release, which adds additional risk. Pin to the exact version or a semver range already validated in this repo.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/package.json` at line 27, Replace the non-deterministic dependency entry for "nitro": "latest" in package.json with a concrete, tested version or semver range (e.g. the resolved 3.0.1-alpha.2 or a validated stable range) to ensure reproducible installs; update the "nitro" value to the chosen pinned version string and run your lockfile install to capture the locked version in pnpm-lock.yaml.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/nxus-editor/package.json`:
- Around line 13-36: The package.json for the app is missing the direct "shadcn"
dependency; update package.json's "dependencies" object to add "shadcn":
"^3.6.2" so imports like "shadcn/tailwind.css" resolve in isolated installs;
ensure the dependency is added alongside the other entries and the version
matches ^3.6.2.
In `@apps/nxus-editor/src/stores/outline.store.test.ts`:
- Line 202: The test description string contains a typo: change the it()
description from "uncolllapses the new parent" to "uncollapses the new parent"
in the test block whose current header is it('uncolllapses the new parent', ()
=> { so the test name is spelled correctly.
---
Nitpick comments:
In `@apps/nxus-editor/package.json`:
- Line 27: Replace the non-deterministic dependency entry for "nitro": "latest"
in package.json with a concrete, tested version or semver range (e.g. the
resolved 3.0.1-alpha.2 or a validated stable range) to ensure reproducible
installs; update the "nitro" value to the chosen pinned version string and run
your lockfile install to capture the locked version in pnpm-lock.yaml.
In `@apps/nxus-editor/src/stores/outline.store.test.ts`:
- Around line 157-183: Add a test that verifies activeNodeId is updated when the
deleted node was active: after seedStore(), call activateNode('b', 0) and assert
activeNodeId === 'b', then call deleteNode('b') and assert the node is removed
(nodes.has('b') is false) and activeNodeId equals the previous visible node
(e.g., 'a2'); place this alongside the other deleteNode tests so deleteNode,
activateNode, activeNodeId, seedStore and WORKSPACE_ROOT_ID are exercised
together.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1751de59-fe9a-40ce-b82a-66672b7baf7f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
apps/nxus-editor/package.jsonapps/nxus-editor/src/lib/supertag-colors.test.tsapps/nxus-editor/src/stores/outline.store.test.tsapps/nxus-editor/vitest.config.ts
| "dependencies": { | ||
| "@fontsource-variable/outfit": "^5.2.8", | ||
| "@nxus/db": "workspace:*", | ||
| "@nxus/ui": "workspace:*", | ||
| "@phosphor-icons/react": "^2.1.10", | ||
| "@tailwindcss/vite": "^4.0.6", | ||
| "@tanstack/react-query": "^5.90.12", | ||
| "@tanstack/react-router": "^1.132.0", | ||
| "@tanstack/react-start": "^1.132.0", | ||
| "@tanstack/router-plugin": "^1.132.0", | ||
| "better-sqlite3": "^12.6.0", | ||
| "class-variance-authority": "^0.7.1", | ||
| "clsx": "^2.1.1", | ||
| "drizzle-orm": "^0.45.1", | ||
| "nitro": "latest", | ||
| "react": "^19.2.0", | ||
| "react-dom": "^19.2.0", | ||
| "tailwind-merge": "^3.4.0", | ||
| "tailwindcss": "^4.0.6", | ||
| "tw-animate-css": "^1.4.0", | ||
| "vite-tsconfig-paths": "^5.1.4", | ||
| "zod": "^4.2.1", | ||
| "zustand": "^5.0.9" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json
from pathlib import Path
for path in sorted(Path("apps").glob("*/package.json")):
data = json.loads(path.read_text())
deps = data.get("dependencies", {})
print(f"{path}: name={data.get('name')} shadcn={deps.get('shadcn', 'MISSING')}")
PYRepository: popemkt/nxus
Length of output: 463
Add the required direct shadcn dependency.
This manifest is missing shadcn, so installs with isolated dependency graphs can fail once the app imports shadcn/tailwind.css. Add it as a direct dependency with version ^3.6.2 to match the other app manifests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/nxus-editor/package.json` around lines 13 - 36, The package.json for the
app is missing the direct "shadcn" dependency; update package.json's
"dependencies" object to add "shadcn": "^3.6.2" so imports like
"shadcn/tailwind.css" resolve in isolated installs; ensure the dependency is
added alongside the other entries and the version matches ^3.6.2.
| expect(useOutlineStore.getState().nodes.get('a')?.parentId).toBe(WORKSPACE_ROOT_ID) | ||
| }) | ||
|
|
||
| it('uncolllapses the new parent', () => { |
There was a problem hiding this comment.
Typo: "uncolllapses" → "uncollapses".
Minor spelling error in the test description.
📝 Proposed fix
- it('uncolllapses the new parent', () => {
+ it('uncollapses the new parent', () => {📝 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.
| it('uncolllapses the new parent', () => { | |
| it('uncollapses the new parent', () => { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/nxus-editor/src/stores/outline.store.test.ts` at line 202, The test
description string contains a typo: change the it() description from
"uncolllapses the new parent" to "uncollapses the new parent" in the test block
whose current header is it('uncolllapses the new parent', () => { so the test
name is spelled correctly.
Field values from DB can be objects (e.g. {filters: ...}), not just
strings. Add safety check: if value is an object, render as JsonField
with a truncated JSON preview and hover tooltip instead of trying to
render it as a React child.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolve package.json conflict: keep predev script from main and editor-app addition to dev script from feature branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reusable /merge-and-ship command that automates: merge main, resolve conflicts, push, create/update PR, monitor CI checks, address review comments, and get PR to ready state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add nxus-editor to expected count (4→5), add 'notepad' to valid icons, and add test case for the editor mini-app entry. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add Notepad import and iconMap entry across all 11 gateway visual components to prevent SSR crash from undefined icon component - Scope editor breadcrumb e2e selectors to .breadcrumbs container to avoid strict mode violation from multiple 'Home' text matches Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| server: { | ||
| watch: { | ||
| ignored: [ | ||
| '**/packages/repos/**', | ||
| '**/.git/**', | ||
| '**/node_modules/**', | ||
| '**/.turbo/**', | ||
| '**/dist/**', | ||
| '**/build/**', | ||
| ], | ||
| }, |
There was a problem hiding this comment.
🔴 Missing server.port and server.strictPort in vite.config.ts breaks gateway proxy pattern
Every other app in the monorepo sets port and strictPort: true in the Vite server config (see apps/nxus-recall/vite.config.ts:20-21, apps/nxus-core/vite.config.ts:27-28). The editor app only passes --port 3005 via the CLI in package.json, but omits strictPort: true from vite.config.ts. Without strictPort, if port 3005 is already in use, Vite silently falls back to another port, which breaks the gateway proxy at apps/nxus-gateway/vite.config.ts:21 that routes /editor to localhost:3005. This violates the established architecture pattern documented in AGENTS.md's Gateway Proxy Architecture section.
| server: { | |
| watch: { | |
| ignored: [ | |
| '**/packages/repos/**', | |
| '**/.git/**', | |
| '**/node_modules/**', | |
| '**/.turbo/**', | |
| '**/dist/**', | |
| '**/build/**', | |
| ], | |
| }, | |
| server: { | |
| port: 3005, | |
| strictPort: true, | |
| watch: { | |
| ignored: [ | |
| '**/packages/repos/**', | |
| '**/.git/**', | |
| '**/node_modules/**', | |
| '**/.turbo/**', | |
| '**/dist/**', | |
| '**/build/**', | |
| ], | |
| }, | |
| }, |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const handleKeyDown = useCallback( | ||
| (e: React.KeyboardEvent<HTMLDivElement>) => { | ||
| const sel = window.getSelection() | ||
|
|
||
| if (e.key === 'Enter' && !e.shiftKey) { | ||
| e.preventDefault() | ||
| createNodeAfter(nodeId) | ||
| return | ||
| } |
There was a problem hiding this comment.
🔴 Escape key does not deactivate the active node editor
When a node is being actively edited (contentEditable is focused), pressing Escape does nothing. The handleKeyDown in apps/nxus-editor/src/components/outline/node-block.tsx:70-187 handles Enter, Tab, Backspace, ArrowUp, and ArrowDown but has no Escape handler. The global handleGlobalKeyDown in apps/nxus-editor/src/components/outline/outline-editor.tsx:126 returns early with if (activeNodeId) return, so it never reaches the Escape handler on line 174. The PRD marks Escape → Deactivate as ✅, and the e2e test at e2e/editor/outline-editor.spec.ts:121 expects this to work, but it will fail.
| const handleKeyDown = useCallback( | |
| (e: React.KeyboardEvent<HTMLDivElement>) => { | |
| const sel = window.getSelection() | |
| if (e.key === 'Enter' && !e.shiftKey) { | |
| e.preventDefault() | |
| createNodeAfter(nodeId) | |
| return | |
| } | |
| const handleKeyDown = useCallback( | |
| (e: React.KeyboardEvent<HTMLDivElement>) => { | |
| const sel = window.getSelection() | |
| if (e.key === 'Escape') { | |
| e.preventDefault() | |
| useOutlineStore.getState().selectNode(nodeId) | |
| return | |
| } | |
| if (e.key === 'Enter' && !e.shiftKey) { | |
| e.preventDefault() | |
| createNodeAfter(nodeId) | |
| return | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
Guard both Backspace paths (empty node delete and merge-with-previous) to only operate on leaf nodes, preventing accidental subtree deletion. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
apps/nxus-editor/src/components/outline/field-value.tsx (3)
226-231:⚠️ Potential issue | 🟠 MajorDate-only strings will display the wrong day in US time zones.
new Date(String(value))treatsYYYY-MM-DDas UTC. For a stored value like2024-03-15, users in US time zones can seeMar 14, 2024here. Parse date-only strings into a localDatebefore formatting.🗓️ Minimal fix
- const displayDate = value - ? new Date(String(value)).toLocaleDateString('en-US', { - month: 'short', - day: 'numeric', - year: 'numeric', - }) - : null + const displayDate = value + ? (() => { + const raw = String(value) + const date = + /^\d{4}-\d{2}-\d{2}$/.test(raw) + ? new Date( + Number(raw.slice(0, 4)), + Number(raw.slice(5, 7)) - 1, + Number(raw.slice(8, 10)), + ) + : new Date(raw) + + return Number.isNaN(date.getTime()) + ? raw + : date.toLocaleDateString('en-US', { + month: 'short', + day: 'numeric', + year: 'numeric', + }) + })() + : nullIn JavaScript, how is `new Date('2024-03-15')` interpreted, and why can `toLocaleDateString()` show March 14 in US time zones?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/components/outline/field-value.tsx` around lines 226 - 231, The code computing displayDate using new Date(String(value)) treats ISO date-only strings like "YYYY-MM-DD" as UTC, causing off-by-one day in US time zones; update the logic where displayDate is derived (the displayDate constant that reads value) to detect date-only strings (e.g. /^\d{4}-\d{2}-\d{2}$/) and parse them into a local Date (construct with new Date(year, monthIndex, day)) before calling toLocaleDateString, while preserving the existing fallback for other value formats.
20-21:⚠️ Potential issue | 🟠 MajorEmpty numeric fields currently round-trip as
0.
Number(value ?? 0)removes the empty state on initial render, andNumber(draft)turns a cleared number input back into0. That makes “unset” indistinguishable from zero and prevents users from actually clearing the field.In JavaScript, what does `Number('')` return, and what pattern is recommended for preserving an empty state when a user clears an `<input type="number">`?Also applies to: 101-120, 144-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/components/outline/field-value.tsx` around lines 20 - 21, The number case currently forces empty numeric inputs to 0 by using Number(value ?? 0); update the logic in the case 'number' branch so you preserve the empty state (pass an empty string or undefined through instead of coercing to 0) and only convert to a number when the input actually contains digits (handle conversion in the NumberField onChange or wrap the value as value === '' ? '' : Number(value)); modify the value prop passed to NumberField and the onChange handling so clearing the input produces an empty value (or undefined) rather than 0.
372-381:⚠️ Potential issue | 🟠 MajorExisting email values have no path back into edit mode.
Once
valueis populated, this branch only renders themailto:link. Users can no longer update or clear an email afterward, even thoughUrlFieldalready exposes the needed edit affordance just above.✏️ Minimal fix
if (value) { return ( - <a - href={`mailto:${value}`} - className="truncate rounded-sm px-1 text-[13px] text-primary/70 underline decoration-primary/30 hover:text-primary" - onClick={(e) => e.stopPropagation()} - > - {String(value)} - </a> + <span className="flex items-center gap-1"> + <a + href={`mailto:${value}`} + className="truncate rounded-sm px-1 text-[13px] text-primary/70 underline decoration-primary/30 hover:text-primary" + onClick={(e) => e.stopPropagation()} + > + {String(value)} + </a> + <button + type="button" + className="text-[11px] text-foreground/30 hover:text-foreground/60" + onClick={(e) => { + e.stopPropagation() + setDraft(String(value)) + setEditing(true) + }} + > + edit + </button> + </span> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-editor/src/components/outline/field-value.tsx` around lines 372 - 381, When an email value is present the code only renders a mailto link and removes any way to edit/clear it; modify the branch in field-value.tsx that renders the mailto anchor so it also exposes the same edit affordance as the UrlField branch (either render the UrlField component with the existing props when value exists or add a small edit/clear button that calls the same edit handler), ensure the edit control stops event propagation like the anchor (onClick={(e)=>e.stopPropagation()}) and reuses the same handlers/state used by UrlField so users can open edit mode and clear the email.
🧹 Nitpick comments (1)
apps/nxus-gateway/src/components/visuals/terminal-cards.tsx (1)
2-11: LGTM — Notepad icon correctly added to support the new editor app.The import and iconMap entry are consistent with the MiniApp interface's
iconunion type, which provides compile-time safety.Consider extracting the shared
iconMapto a common module (e.g.,@/config/icons.ts) to reduce duplication across the 10+ visual card components. This would make future icon additions a single-file change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/nxus-gateway/src/components/visuals/terminal-cards.tsx` around lines 2 - 11, Extract the duplicated iconMap into a shared module and update this component to import it: create a new module that exports the iconMap (mapping the same symbols Cube, Graph, CalendarBlank, Brain, Notepad) and any related type compatibility with MiniApp, then replace the local iconMap and the local imports by importing the shared iconMap (and the icon components only from that module if you prefer) in terminal-cards.tsx; also update the other visual card components to import the same shared iconMap so future icon additions are made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/commands/merge-and-ship.md:
- Around line 29-32: Replace the interactive commit step that uses the plain
"git commit" command with a non-interactive commit by changing the instruction
to use "git commit --no-edit" so the generated merge message is preserved and
the operation does not open an editor or block automation; update the line
containing the "git commit" example in the merge-and-ship instructions
accordingly.
- Around line 77-80: The gh api call uses a literal {number} token which won't
be expanded; replace it by interpolating the actual PR number variable into the
endpoint path so the request targets the real pull request (i.e., stop using "gh
api repos/{owner}/{repo}/pulls/{number}/comments" and instead build the path
with the PR number variable used in your script, e.g. substitute {number} with
your PR_NUMBER variable when constructing the endpoint before calling gh api);
ensure any code that derives the PR ID (e.g., where you set or parse PR_NUMBER)
is executed prior to this API call.
In `@apps/nxus-editor/src/components/outline/field-value.tsx`:
- Around line 11-38: FieldValue is accepting value: unknown and coercing it
inside the switch (Boolean(value), Number(...), String(...)) which silently
masks malformed inputs; define Zod schemas for each field type (e.g.,
booleanSchema, numberSchema, dateSchema, urlSchema, emailSchema, selectSchema,
nodeSchema, nodesSchema, jsonSchema, textSchema), validate/parse the incoming
value at the top of the FieldValue function (before the switch) using the
appropriate schema based on fieldType (call schema.parse(value)), handle parse
failures by returning a safe error/invalid UI (or JsonField with error info) and
then pass the parsed/typed value into the corresponding render branches
(BooleanField, NumberField, DateField, UrlField, EmailField, SelectField,
NodeRefField, NodeRefsField, JsonField, TextField) so downstream components
receive correctly-typed values.
In `@apps/nxus-gateway/src/config/mini-apps.test.ts`:
- Around line 42-48: Add a TanStack Router route file named editor.tsx that
registers the '/editor' path and exports the route component (e.g., EditorRoute
or default Editor component) so the route tree includes the editor route; ensure
the route's path/id matches the mini-app id 'nxus-editor' used in the tests and
that this new route is wired into the root route tree (imported or discovered by
your file-based routing) so navigation to /editor succeeds.
In `@e2e/editor/outline-editor.spec.ts`:
- Around line 84-97: The tests named "clicking outside deactivates the editor",
"Escape ... enters selection mode", and "Arrow keys navigate ..." are missing
assertions that verify the actual UI state changes; after clicking the outline
background in the "clicking outside deactivates the editor" test (locator
'.outline-body') assert that no element with [contenteditable="true"] is visible
and that the previously focused node (located via '.node-content' and its
data-node-id attribute exposed by node-block.tsx) does not have the selection
styling; in the "Escape ... enters selection mode" test, after sending Escape
assert the selected node by checking the element with the expected data-node-id
has the selection class/state; and in "Arrow keys navigate ..." after sending
arrow keys assert the focused/selected node changed by verifying the
corresponding [data-node-id="..."] element has selection styling or a11y focus,
using the data-node-id attribute and selection class from
apps/nxus-editor/src/components/outline/node-block.tsx to locate and assert the
correct node.
- Line 27: Replace the unconditional page.waitForTimeout(...) calls with
state-based waits: remove the page.waitForTimeout(2000) usages and instead wait
for a specific locator or condition using Playwright's auto-waiting APIs (e.g.,
locator.waitFor(), expect(locator).toBeVisible()/toHaveText()/toBeHidden(), or
page.waitForResponse()/waitForSelector) so the tests for outline-editor.spec.ts
proceed as soon as the UI is ready; update each occurrence of
page.waitForTimeout to target the relevant element or empty-state assertion for
that test scenario to make the suite deterministic and faster.
---
Duplicate comments:
In `@apps/nxus-editor/src/components/outline/field-value.tsx`:
- Around line 226-231: The code computing displayDate using new
Date(String(value)) treats ISO date-only strings like "YYYY-MM-DD" as UTC,
causing off-by-one day in US time zones; update the logic where displayDate is
derived (the displayDate constant that reads value) to detect date-only strings
(e.g. /^\d{4}-\d{2}-\d{2}$/) and parse them into a local Date (construct with
new Date(year, monthIndex, day)) before calling toLocaleDateString, while
preserving the existing fallback for other value formats.
- Around line 20-21: The number case currently forces empty numeric inputs to 0
by using Number(value ?? 0); update the logic in the case 'number' branch so you
preserve the empty state (pass an empty string or undefined through instead of
coercing to 0) and only convert to a number when the input actually contains
digits (handle conversion in the NumberField onChange or wrap the value as value
=== '' ? '' : Number(value)); modify the value prop passed to NumberField and
the onChange handling so clearing the input produces an empty value (or
undefined) rather than 0.
- Around line 372-381: When an email value is present the code only renders a
mailto link and removes any way to edit/clear it; modify the branch in
field-value.tsx that renders the mailto anchor so it also exposes the same edit
affordance as the UrlField branch (either render the UrlField component with the
existing props when value exists or add a small edit/clear button that calls the
same edit handler), ensure the edit control stops event propagation like the
anchor (onClick={(e)=>e.stopPropagation()}) and reuses the same handlers/state
used by UrlField so users can open edit mode and clear the email.
---
Nitpick comments:
In `@apps/nxus-gateway/src/components/visuals/terminal-cards.tsx`:
- Around line 2-11: Extract the duplicated iconMap into a shared module and
update this component to import it: create a new module that exports the iconMap
(mapping the same symbols Cube, Graph, CalendarBlank, Brain, Notepad) and any
related type compatibility with MiniApp, then replace the local iconMap and the
local imports by importing the shared iconMap (and the icon components only from
that module if you prefer) in terminal-cards.tsx; also update the other visual
card components to import the same shared iconMap so future icon additions are
made in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54c84a06-3e3c-431d-9928-a5376a5f5699
📒 Files selected for processing (20)
.claude/commands/merge-and-ship.mdapps/nxus-editor/src/components/outline/field-value.tsxapps/nxus-editor/src/components/outline/node-block.tsxapps/nxus-gateway/src/components/visuals/aurora-cards.tsxapps/nxus-gateway/src/components/visuals/bento-grid-cards.tsxapps/nxus-gateway/src/components/visuals/blueprint-cards.tsxapps/nxus-gateway/src/components/visuals/cosmic-tarot-cards.tsxapps/nxus-gateway/src/components/visuals/dashboard-cards.tsxapps/nxus-gateway/src/components/visuals/glass-3d-cards.tsxapps/nxus-gateway/src/components/visuals/minimal-zen-cards.tsxapps/nxus-gateway/src/components/visuals/orbital-cards.tsxapps/nxus-gateway/src/components/visuals/spatial-cards.tsxapps/nxus-gateway/src/components/visuals/terminal-cards.tsxapps/nxus-gateway/src/config/mini-apps.test.tsapps/nxus-gateway/src/routes/index.tsxapps/nxus-gateway/vite.config.tse2e/editor/outline-editor.spec.tse2e/fixtures/base.fixture.tse2e/helpers/navigation.tspackage.json
✅ Files skipped from review due to trivial changes (2)
- apps/nxus-gateway/src/components/visuals/bento-grid-cards.tsx
- apps/nxus-gateway/src/components/visuals/orbital-cards.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/nxus-gateway/vite.config.ts
- apps/nxus-editor/src/components/outline/node-block.tsx
- package.json
| After all conflicts resolved: | ||
| ```bash | ||
| git commit | ||
| ``` |
There was a problem hiding this comment.
Use a non-interactive merge commit here.
A bare git commit will open the editor after conflict resolution and can block the slash command. git commit --no-edit keeps the generated merge message and preserves automation.
🤖 Minimal fix
- git commit
+ git commit --no-edit📝 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.
| After all conflicts resolved: | |
| ```bash | |
| git commit | |
| ``` | |
| After all conflicts resolved: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/commands/merge-and-ship.md around lines 29 - 32, Replace the
interactive commit step that uses the plain "git commit" command with a
non-interactive commit by changing the instruction to use "git commit --no-edit"
so the generated merge message is preserved and the operation does not open an
editor or block automation; update the line containing the "git commit" example
in the merge-and-ship instructions accordingly.
| Also check inline comments: | ||
| ```bash | ||
| gh api repos/{owner}/{repo}/pulls/{number}/comments | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In GitHub CLI gh api, which endpoint placeholders are expanded automatically? Does repos/{owner}/{repo}/pulls/{number}/commentssubstitute{number} on its own?
💡 Result:
In gh api, the only endpoint-argument placeholders that GitHub CLI expands automatically are:
{owner}{repo}{branch}
They’re filled from the repo detected in the current directory (or from GH_REPO). No other {...} tokens are expanded in the endpoint path. So repos/{owner}/{repo}/pulls/{number}/comments will not substitute {number} automatically—you must provide the PR number yourself (e.g. .../pulls/123/comments). [1]
Source: [1]
Capture the PR number and interpolate it into the endpoint path.
gh api only auto-expands {owner}, {repo}, and {branch} placeholders. The {number} token will be passed literally, causing the endpoint to fail when fetching inline comments.
🛠️ Fix
- gh api repos/{owner}/{repo}/pulls/{number}/comments
+ PR_NUMBER="$(gh pr view --json number --jq '.number')"
+ gh api "repos/{owner}/{repo}/pulls/${PR_NUMBER}/comments"📝 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.
| Also check inline comments: | |
| ```bash | |
| gh api repos/{owner}/{repo}/pulls/{number}/comments | |
| ``` | |
| Also check inline comments: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/commands/merge-and-ship.md around lines 77 - 80, The gh api call
uses a literal {number} token which won't be expanded; replace it by
interpolating the actual PR number variable into the endpoint path so the
request targets the real pull request (i.e., stop using "gh api
repos/{owner}/{repo}/pulls/{number}/comments" and instead build the path with
the PR number variable used in your script, e.g. substitute {number} with your
PR_NUMBER variable when constructing the endpoint before calling gh api); ensure
any code that derives the PR ID (e.g., where you set or parse PR_NUMBER) is
executed prior to this API call.
| export function FieldValue({ fieldType, value, onChange }: FieldValueProps) { | ||
| // Safety: if value is an object/array and not a primitive type, render as JSON | ||
| if (value !== null && value !== undefined && typeof value === 'object' && fieldType !== 'nodes') { | ||
| return <JsonField value={value} /> | ||
| } | ||
|
|
||
| switch (fieldType) { | ||
| case 'boolean': | ||
| return <BooleanField value={Boolean(value)} onChange={onChange} /> | ||
| case 'number': | ||
| return <NumberField value={Number(value ?? 0)} onChange={onChange} /> | ||
| case 'date': | ||
| return <DateField value={String(value ?? '')} onChange={onChange} /> | ||
| case 'url': | ||
| return <UrlField value={String(value ?? '')} onChange={onChange} /> | ||
| case 'email': | ||
| return <EmailField value={String(value ?? '')} onChange={onChange} /> | ||
| case 'select': | ||
| return <SelectField value={String(value ?? '')} /> | ||
| case 'node': | ||
| return <NodeRefField value={String(value ?? '')} /> | ||
| case 'nodes': | ||
| return <NodeRefsField values={Array.isArray(value) ? value : []} /> | ||
| case 'json': | ||
| return <JsonField value={value} /> | ||
| case 'text': | ||
| default: | ||
| return <TextField value={String(value ?? '')} onChange={onChange} /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'value:\s*unknown|z\.unknown\(\)|Boolean\(value\)|Number\(value|String\(value' \
apps/nxus-editor/src/components/outline/field-value.tsx \
apps/nxus-editor/src/components/outline/fields-section.tsx \
apps/nxus-editor/src/services/outline.server.tsRepository: popemkt/nxus
Length of output: 3488
Define a Zod schema for each field type and validate the incoming value before the switch statement.
FieldValue receives value as unknown type (line 7) from fields-section.tsx, which pulls from outline.server.ts where the service accepts value: z.unknown() (line 293). The coercions in the switch cases (Boolean(value), Number(value ?? 0), String(value ?? '')) silently turn malformed payloads into plausible UI state instead of failing fast. Define field-type-specific Zod schemas, validate the value at the component entry point, and only then branch on fieldType with properly typed values. Per coding guidelines: "Use Zod schemas with parse() for validating external data instead of assuming validity" and "Validate external inputs using Zod schemas before processing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/nxus-editor/src/components/outline/field-value.tsx` around lines 11 -
38, FieldValue is accepting value: unknown and coercing it inside the switch
(Boolean(value), Number(...), String(...)) which silently masks malformed
inputs; define Zod schemas for each field type (e.g., booleanSchema,
numberSchema, dateSchema, urlSchema, emailSchema, selectSchema, nodeSchema,
nodesSchema, jsonSchema, textSchema), validate/parse the incoming value at the
top of the FieldValue function (before the switch) using the appropriate schema
based on fieldType (call schema.parse(value)), handle parse failures by
returning a safe error/invalid UI (or JsonField with error info) and then pass
the parsed/typed value into the corresponding render branches (BooleanField,
NumberField, DateField, UrlField, EmailField, SelectField, NodeRefField,
NodeRefsField, JsonField, TextField) so downstream components receive
correctly-typed values.
| it('contains nxus-editor', () => { | ||
| const editor = miniApps.find((app) => app.id === 'nxus-editor') | ||
| expect(editor).toBeDefined() | ||
| expect(editor!.name).toBe('nXus Editor') | ||
| expect(editor!.path).toBe('/editor') | ||
| expect(editor!.icon).toBe('notepad') | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that /editor route is registered in TanStack Router
# Check for editor route registration in route files
echo "=== Searching for /editor route registration ==="
rg -n "editor" --type ts -g '*route*' apps/nxus-gateway/
echo ""
echo "=== Checking vite proxy configuration for /editor ==="
rg -n "editor" apps/nxus-gateway/vite.config.ts
echo ""
echo "=== Listing route tree structure ==="
fd -e ts -e tsx route apps/nxus-gateway/src/Repository: popemkt/nxus
Length of output: 324
🏁 Script executed:
# Check if editor route is in the route tree
echo "=== Checking routeTree.gen.ts for editor route ==="
grep -n "editor" apps/nxus-gateway/src/routeTree.gen.ts
echo ""
echo "=== Checking router.tsx for editor route ==="
grep -n "editor" apps/nxus-gateway/src/router.tsx
echo ""
echo "=== Finding route definition files ==="
find apps/nxus-gateway/src -name "*.route.*" -o -name "*route*.tsx" -o -name "*route*.ts" | grep -v node_modules | head -20Repository: popemkt/nxus
Length of output: 264
🏁 Script executed:
# Get file sizes and check content
echo "=== Size of routeTree.gen.ts ==="
wc -l apps/nxus-gateway/src/routeTree.gen.ts
echo ""
echo "=== First 50 lines of routeTree.gen.ts ==="
head -50 apps/nxus-gateway/src/routeTree.gen.ts
echo ""
echo "=== Size and content of router.tsx ==="
wc -l apps/nxus-gateway/src/router.tsx
cat -n apps/nxus-gateway/src/router.tsxRepository: popemkt/nxus
Length of output: 2763
🏁 Script executed:
# Check if there's an editor route file that should exist
echo "=== Looking for route files structure ==="
find apps/nxus-gateway/src/routes -type f 2>/dev/null | head -20
echo ""
echo "=== Checking root route file ==="
cat apps/nxus-gateway/src/routes/__root.tsx 2>/dev/null | head -30
echo ""
echo "=== Checking if editor.tsx route file exists ==="
ls -la apps/nxus-gateway/src/routes/editor* 2>/dev/null || echo "No editor route file found"Repository: popemkt/nxus
Length of output: 913
The nxus-editor route is not registered in TanStack Router.
While the test correctly validates the mini-app configuration, the /editor route itself is missing from the route tree. The route structure contains only __root__.tsx and index.tsx, and there is no editor.tsx route file. Following TanStack Router's file-based routing pattern, create apps/nxus-gateway/src/routes/editor.tsx to register the route and enable navigation to /editor.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/nxus-gateway/src/config/mini-apps.test.ts` around lines 42 - 48, Add a
TanStack Router route file named editor.tsx that registers the '/editor' path
and exports the route component (e.g., EditorRoute or default Editor component)
so the route tree includes the editor route; ensure the route's path/id matches
the mini-app id 'nxus-editor' used in the tests and that this new route is wired
into the root route tree (imported or discovered by your file-based routing) so
navigation to /editor succeeds.
| test.describe('Node Rendering', () => { | ||
| test('displays node blocks from database', async ({ page }) => { | ||
| // Wait for loading to finish | ||
| await page.waitForTimeout(2000) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Playwright recommend using page.waitForTimeout() in tests, or waiting on locators/assertions instead?
💡 Result:
Playwright discourages using page.waitForTimeout() in tests: the docs say it should only be used for debugging, and that tests which “wait for time are inherently flaky.” Instead, you should rely on locator actions and “web-first” assertions (which auto-wait and retry until conditions are met) or other explicit signals (like network events). [1], [2]
🏁 Script executed:
# First, check the test file structure and content
head -100 e2e/editor/outline-editor.spec.tsRepository: popemkt/nxus
Length of output: 3785
🏁 Script executed:
# Look for existing selectors/locators in the component file mentioned in the scratchpad
find . -path "*/outline/node-block.tsx" -o -path "*/outline*" -name "*.tsx" | head -10Repository: popemkt/nxus
Length of output: 462
🏁 Script executed:
# Search for data-node-id or outline-editor selectors in the codebase
rg "outline-editor|data-node-id|node-block" --type tsx --type ts | head -20Repository: popemkt/nxus
Length of output: 83
🏁 Script executed:
# Use rg without the type flag - just search for the pattern
rg "outline-editor|data-node-id|node-block" | head -30Repository: popemkt/nxus
Length of output: 1220
🏁 Script executed:
# Check the rest of the test file to verify all the line numbers mentioned in "Also applies to"
cat -n e2e/editor/outline-editor.spec.ts | sed -n '20,280p'Repository: popemkt/nxus
Length of output: 11547
🏁 Script executed:
# Look at the node-block component to see what selectors are available
cat apps/nxus-editor/src/components/outline/node-block.tsxRepository: popemkt/nxus
Length of output: 7316
Replace fixed sleeps with state-based waits.
This pattern adds roughly 30 seconds of unconditional delay to the suite and leaves readiness timing-dependent. Playwright's web-first assertions and locator actions auto-wait and retry until conditions are met, making tests faster and more reliable than page.waitForTimeout().
⏱️ Example pattern
- await page.waitForTimeout(2000)
+ await expect(page.locator('.node-block, .outline-body')).toBeVisible({ timeout: 10_000 })Or for tests expecting empty states:
- await page.waitForTimeout(2000)
+ await expect(
+ page.locator('.node-block').or(page.getByText('Empty')).or(page.getByText('No nodes found'))
+ ).toBeVisible({ timeout: 10_000 })Also applies to: 42-42, 56-56, 68-68, 85-85, 102-102, 122-122, 141-141, 164-164, 195-195, 226-226, 249-249, 267-267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/editor/outline-editor.spec.ts` at line 27, Replace the unconditional
page.waitForTimeout(...) calls with state-based waits: remove the
page.waitForTimeout(2000) usages and instead wait for a specific locator or
condition using Playwright's auto-waiting APIs (e.g., locator.waitFor(),
expect(locator).toBeVisible()/toHaveText()/toBeHidden(), or
page.waitForResponse()/waitForSelector) so the tests for outline-editor.spec.ts
proceed as soon as the UI is ready; update each occurrence of
page.waitForTimeout to target the relevant element or empty-state assertion for
that test scenario to make the suite deterministic and faster.
- Fix generateOrderBetween collision when gap ≤ 1 by extending with fractional digits instead of producing duplicate orders - Persist both sides of order swap in moveNodeUp/Down to prevent server state drift after page reload - Use atomic functional setState for temp-to-real ID reconciliation to prevent concurrent edit loss - Guard outer div onClick with e.target === e.currentTarget to stop click bubbling from children clearing active node state - Remove Tab preventDefault in selection mode to allow native focus traversal for keyboard accessibility Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix content timer race: cancel pending debounce for temp ID on server
ID swap to prevent stale writes
- Add optimistic field value updates: fields-section now updates Zustand
store before server call, matching the pattern used by all other mutations
- Fix type safety: replace `{} | null` with `unknown` and remove unsafe
cast in outline.server.ts field value mapping
- Make deleteNode a pure data operation: remove implicit selection
side-effects from store, let callers (node-block, outline-editor)
explicitly own selection after deletion
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
||
| function generateOrderBetween(a: string | null, b: string | null): string { | ||
| if (!a && !b) return generateOrder(500_000) | ||
| if (!a) return generateOrder(Math.floor(parseInt(b!, 10) / 2)) | ||
| if (!b) return generateOrder(parseInt(a, 10) + 1000) | ||
| const aNum = parseInt(a, 10) | ||
| const bNum = parseInt(b, 10) | ||
| const gap = bNum - aNum | ||
| if (gap > 1) { | ||
| return generateOrder(aNum + Math.floor(gap / 2)) | ||
| } | ||
| // Gap exhausted — extend by appending fractional digits | ||
| return a + '5'.padStart(4, '0') |
There was a problem hiding this comment.
🔴 generateOrderBetween produces orders that sort outside [a, b] when orders have mixed string lengths
When the gap between a and b is exhausted (≤1), the function appends fractional digits (a + '0005'), creating a 12+ character order string. On subsequent calls where one argument is an 8-char string and the other is a 12+ char extended string, parseInt treats the extended string as a huge integer (e.g., parseInt('000010010005', 10) = 10010005) while the 8-char string stays small (e.g., parseInt('00001000', 10) = 1000). The gap calculation then produces a large positive value, and generateOrder(...) emits an 8-char string that doesn't sort correctly between the two inputs under localeCompare.
Concrete reproduction via outdentNode
If parentOrder = '00001000' and nextUncleOrder = '000010010005' (extended from a prior fractional insertion):
aNum = 1000,bNum = 10010005,gap = 10009005 > 1- Returns
generateOrder(1000 + 5004502)='05005502' '05005502'.localeCompare('000010010005')→ positive (result > b)- The outdented node sorts AFTER the next uncle instead of between parent and uncle, corrupting sibling order.
Prompt for agents
In apps/nxus-editor/src/stores/outline.store.ts, the generateOrderBetween function (lines 39-51) mixes parseInt-based integer arithmetic with variable-length string comparison (localeCompare). This breaks when one order string has been extended by the fractional fallback (12+ chars) and the other is a standard 8-char string.
The fix should make the order generation consistent with string-based comparison. Options:
1. Always use the string-append approach: find the common prefix of a and b, then generate a midpoint string character-by-character. Libraries like `fractional-indexing` do this correctly.
2. Alternatively, normalize both strings to the same length (pad the shorter one with '0's on the right) before doing integer arithmetic, and ensure generateOrder pads to match the longest input length rather than always 8.
The key invariant is: the returned string must satisfy a < result < b under localeCompare for all possible input pairs (a, b) where a < b.
Was this helpful? React with 👍 or 👎 to provide feedback.
| data: { | ||
| content: '', | ||
| parentId: serverParentId, | ||
| order: parseInt(node.order, 10), |
There was a problem hiding this comment.
🔴 parseInt on extended order strings corrupts sibling order after server roundtrip
The generateOrderBetween function (outline.store.ts:51) appends fractional digits when the gap between siblings is exhausted, producing strings longer than 8 characters (e.g., "000010000005"). Throughout use-outline-sync.ts, these order strings are converted to integers via parseInt(node.order, 10) before being sent to the server. parseInt("000010000005", 10) yields 10000005. On reload, the server at outline.server.ts:98 converts back: String(10000005).padStart(8, '0') → "10000005". Lexicographically, "000010000005" < "00002000" (correct), but "10000005" > "00002000" (wrong). So a node intended to sort between siblings A and B now sorts after B on page reload, silently corrupting document order. This triggers after ~10 rapid insertions between the same pair of siblings.
Affected parseInt call sites in use-outline-sync.ts
Line 71: order: parseInt(node.order, 10) (createNodeAfter)
Line 157: order: parseInt(node.order, 10) (indentNode)
Line 176: order: parseInt(node.order, 10) (outdentNode)
Line 202: order: parseInt(node.order, 10) (moveNodeUp)
Line 216: order: parseInt(sib.order, 10) (moveNodeUp sibling)
Line 239: order: parseInt(node.order, 10) (moveNodeDown)
Line 253: order: parseInt(sib.order, 10) (moveNodeDown sibling)
Prompt for agents
The order system uses string-based lexicographic comparison in the client store but persists as integers to the server. When generateOrderBetween produces extended strings (gap exhausted), parseInt truncates the semantic value, causing order corruption on reload.
Two options to fix:
Option A (recommended): Change the server to store order as a string, not an integer. In apps/nxus-editor/src/services/outline.server.ts, change setProperty calls to store the order string directly. Update the Zod schemas for createNodeServerFn, reparentNodeServerFn, and reorderNodeServerFn to accept z.string() for order instead of z.number(). Then in use-outline-sync.ts, pass node.order as-is (string) instead of parseInt.
Option B: Change generateOrderBetween in apps/nxus-editor/src/stores/outline.store.ts to never produce strings longer than 8 chars. Instead of appending fractional digits, renumber siblings when the gap is exhausted. This is more complex but keeps the integer storage model.
Was this helpful? React with 👍 or 👎 to provide feedback.
- Bullet: supertag nodes now show # hashtag instead of diamond shape, regular nodes keep round filled dots, collapsed halo is always round - Fields: render at child indent level (depth+1) with a field icon in the bullet column, fixed-width labels (120px) for value alignment - Node refs: resolve referenced node content from store, render as inline pills with bullet dot, clickable to zoom into referenced node - Consistent 28px row height across node rows, field rows, and inputs to prevent layout shifts between editable and display states Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The # hashtag bullet was incorrectly showing for every node that had supertags attached. In Tana, # is only for supertag definition nodes. Regular nodes always use round dots, colored by primary tag if present. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pass systemId through from AssembledNode.supertags to the outline. Only nodes tagged with supertag:supertag (the meta-supertag) show the # hashtag bullet. Regular nodes with supertags show a colored round dot. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| const prevId = getPreviousVisibleNode(nodeId) | ||
| deleteNode(nodeId) | ||
| if (prevId) { | ||
| const prevNode = nodes.get(prevId) | ||
| activateNode(prevId, prevNode?.content.length ?? 0) | ||
| } |
There was a problem hiding this comment.
🟡 Stale node references in handleKeyDown callback due to closure over nodes Map
In node-block.tsx:71-194, the handleKeyDown callback is memoized with nodes in its dependency array. Since nodes is a Zustand-subscribed Map reference that changes on every store update, handleKeyDown is recreated on every node change — so it's not actually stale in the traditional sense. However, after deleteNode(nodeId) is called at line 100, the code immediately reads nodes.get(prevId) at line 102 using the pre-mutation nodes reference captured in the closure. The deleteNode call triggers an async store update, but the nodes variable in the current closure still points to the old Map. In practice this is unlikely to cause a crash (the prev node data would still be present in the old map), but it means activateNode at line 103 operates on potentially stale content length. Similarly at line 119, after updateNodeContent and deleteNode, activateNode(prevId, mergePos) uses mergePos from the old map — which is actually correct since we want the pre-merge length. This is a minor consistency issue.
Was this helpful? React with 👍 or 👎 to provide feedback.
- Tree line now wraps both fields and children in a single container, so the vertical line extends through field rows like regular children - Field label uses items-start for top alignment on multiline content - Unified font size to 14.5px/1.6 line-height across node text, field labels, and field values — no more size mismatch between areas Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… NodeBlock refs 1. Nodes with only fields are now collapsible — toggleCollapse checks both children.length and fields.length 2. Node rows and field rows use items-center for proper vertical alignment of bullets, icons, labels, and text on the same midline 3. Text-based field values (text, number, url, email) now use contentEditable like NodeContent — same element in both states, no layout shift between display and edit mode 4. Reference field types (node, nodes) render as recursive NodeBlock components at depth+2, treating refs like children of the field Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use items-start on field row so multiline content flows top-to-bottom while field icon (h-6) and label (h-6) stay pinned to the first line. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| server: { | ||
| watch: { | ||
| ignored: [ | ||
| '**/packages/repos/**', | ||
| '**/.git/**', | ||
| '**/node_modules/**', | ||
| '**/.turbo/**', | ||
| '**/dist/**', | ||
| '**/build/**', | ||
| ], | ||
| }, |
There was a problem hiding this comment.
🔴 Missing strictPort: true in editor vite.config.ts breaks gateway proxy when port 3005 is occupied
Every other app in the monorepo (nxus-core, nxus-gateway, nxus-workbench, nxus-calendar, nxus-recall) specifies strictPort: true in their vite.config.ts server config. The editor app at apps/nxus-editor/vite.config.ts:19 omits this. Without strictPort, if port 3005 is already in use, Vite silently picks the next available port. The gateway proxy at apps/nxus-gateway/vite.config.ts:21 is hardcoded to forward /editor requests to localhost:3005, so requests would fail with 502 errors. The port is also missing from the server config (only set via CLI --port 3005 in package.json scripts), unlike other apps which set it in both places.
| server: { | |
| watch: { | |
| ignored: [ | |
| '**/packages/repos/**', | |
| '**/.git/**', | |
| '**/node_modules/**', | |
| '**/.turbo/**', | |
| '**/dist/**', | |
| '**/build/**', | |
| ], | |
| }, | |
| server: { | |
| port: 3005, | |
| strictPort: true, | |
| watch: { | |
| ignored: [ | |
| '**/packages/repos/**', | |
| '**/.git/**', | |
| '**/node_modules/**', | |
| '**/.turbo/**', | |
| '**/dist/**', | |
| '**/build/**', | |
| ], | |
| }, | |
| }, |
Was this helpful? React with 👍 or 👎 to provide feedback.
Field icon clicks navigate to the field definition node, and supertag badge clicks navigate to the supertag definition node, enabling quick exploration of the node graph. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All node navigations (breadcrumbs, bullet cmd+click, field icon click, supertag badge click, unresolved ref click) now push to browser history via TanStack Router search params (?node=<id>), making them bookmarkable and supporting back/forward navigation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Each field type now shows a distinct Phosphor icon instead of the generic field bullet (e.g., calendar for date, toggle for boolean, link for URL, tree for node refs). URL and email fields render as clickable links with external-open/mailto action buttons. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When a field definition node lacks an explicit field_type property (defaulting to 'text'), the server now infers 'node' or 'nodes' type from UUID-shaped values. Also fixes multi-value fields (nodes type) to pass all referenced node IDs instead of only the first. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Node reference fields now render as inline content-only rows (bullet + content + supertags) instead of full recursive NodeBlock components. This prevents infinite loops when nodes reference each other or themselves through fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| function ScrollbarManager() { | ||
| useEffect(() => { | ||
| let timeout: NodeJS.Timeout | ||
|
|
||
| const handleScroll = (e: Event) => { | ||
| const target = e.target | ||
| if (!target || !(target instanceof HTMLElement || target === document)) | ||
| return | ||
|
|
||
| const element = | ||
| target === document ? document.documentElement : (target as HTMLElement) | ||
|
|
||
| element.setAttribute('data-scrolling', 'true') | ||
|
|
||
| clearTimeout(timeout) | ||
| timeout = setTimeout(() => { | ||
| element.removeAttribute('data-scrolling') | ||
| }, 1000) | ||
| } | ||
|
|
||
| window.addEventListener('scroll', handleScroll, true) | ||
| return () => { | ||
| window.removeEventListener('scroll', handleScroll, true) | ||
| clearTimeout(timeout) | ||
| } | ||
| }, []) | ||
|
|
||
| return null | ||
| } |
There was a problem hiding this comment.
🟡 ScrollbarManager uses single timeout across all elements, leaving stale data-scrolling attributes
In ScrollbarManager (apps/nxus-editor/src/routes/__root.tsx:101-129), a single timeout variable is shared for all scroll events from any element. When element A scrolls, it gets data-scrolling='true' and a timeout is set to remove it. If element B scrolls before that timeout fires, clearTimeout(timeout) cancels A's cleanup, and only B's attribute gets removed after 1 second. Element A's data-scrolling='true' persists permanently, causing its scrollbar to stay visible forever. This is triggered when the outline body and any nested scrollable container (or the document) scroll within 1 second of each other.
| function ScrollbarManager() { | |
| useEffect(() => { | |
| let timeout: NodeJS.Timeout | |
| const handleScroll = (e: Event) => { | |
| const target = e.target | |
| if (!target || !(target instanceof HTMLElement || target === document)) | |
| return | |
| const element = | |
| target === document ? document.documentElement : (target as HTMLElement) | |
| element.setAttribute('data-scrolling', 'true') | |
| clearTimeout(timeout) | |
| timeout = setTimeout(() => { | |
| element.removeAttribute('data-scrolling') | |
| }, 1000) | |
| } | |
| window.addEventListener('scroll', handleScroll, true) | |
| return () => { | |
| window.removeEventListener('scroll', handleScroll, true) | |
| clearTimeout(timeout) | |
| } | |
| }, []) | |
| return null | |
| } | |
| function ScrollbarManager() { | |
| useEffect(() => { | |
| const timeouts = new Map<Element, NodeJS.Timeout>() | |
| const handleScroll = (e: Event) => { | |
| const target = e.target | |
| if (!target || !(target instanceof HTMLElement || target === document)) | |
| return | |
| const element = | |
| target === document ? document.documentElement : (target as HTMLElement) | |
| element.setAttribute('data-scrolling', 'true') | |
| const existing = timeouts.get(element) | |
| if (existing) clearTimeout(existing) | |
| timeouts.set(element, setTimeout(() => { | |
| element.removeAttribute('data-scrolling') | |
| timeouts.delete(element) | |
| }, 1000)) | |
| } | |
| window.addEventListener('scroll', handleScroll, true) | |
| return () => { | |
| window.removeEventListener('scroll', handleScroll, true) | |
| for (const t of timeouts.values()) clearTimeout(t) | |
| } | |
| }, []) | |
| return null | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
…yling - Fix text cursor jumping and bad typing performance by removing `content` from useEffect deps in NodeContent (DOM owns text during active editing) - Fix all-NodeBlock re-renders by replacing broad `nodes` Map subscription with `useShallow` selector and `getState()` for event handlers - Fix bullet/text midline alignment: min-h-[28px] → min-h-6 (24px) - Fix supertag badges centering on multiline: items-center → items-start with h-6 on badge container to pin to first line - Remove hover effects from field values for unified plane feel - Fix node ref field alignment with py-1 field rows - Add zoomed-in detail view with root node header, fields, and children - Add editor UX rules documentation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…r, and flaky navigation - Add Escape key handler in node-block to deactivate editor and enter selection mode - Add › indicator to field labels expected by outline-editor E2E test - Pin collapse test locator to data-node-id to prevent re-evaluation after DOM change - Add 502 retry logic in navigateToApp fixture to handle upstream app startup race Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…iolation - Add ensureDatabaseReady() to queryInboxItemsBySystemQuery to prevent race where nodeFacade.init() sets seedAttempted=true before seed callback is registered, causing auto-seeding to be skipped on fresh CI environments - Fix C1 test strict mode violation: .or() chain matched both "No apps found" and "Add apps to get started" as separate elements; simplified to single .or() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| className={cn( | ||
| editableClass, | ||
| 'text-primary/70 underline underline-offset-2 decoration-primary/20', | ||
| 'cursor-text hover:bg-foreground/5 truncate', |
There was a problem hiding this comment.
🔴 hover:bg-foreground/5 on URL field value violates editor-ux-rules.md
The .claude/rules/editor-ux-rules.md rule file explicitly states: "No hover effects on field values — no hover:bg-foreground/5 or similar." However, UrlField at line 311 applies exactly hover:bg-foreground/5 to the field value area. This is a direct violation of a mandatory repository rule.
| 'cursor-text hover:bg-foreground/5 truncate', | |
| 'cursor-text truncate', |
Was this helpful? React with 👍 or 👎 to provide feedback.
| className={cn( | ||
| editableClass, | ||
| 'text-primary/70 underline underline-offset-2 decoration-primary/20', | ||
| 'cursor-text hover:bg-foreground/5 truncate', |
There was a problem hiding this comment.
🔴 hover:bg-foreground/5 on Email field value violates editor-ux-rules.md
Same violation as the URL field. The .claude/rules/editor-ux-rules.md rule file explicitly states: "No hover effects on field values — no hover:bg-foreground/5 or similar." EmailField at line 408 applies hover:bg-foreground/5 to the field value area.
| 'cursor-text hover:bg-foreground/5 truncate', | |
| 'cursor-text truncate', |
Was this helpful? React with 👍 or 👎 to provide feedback.
After nxus.db and surreal.db were removed from git tracking (67ffb43), CI environments start with empty databases. The auto-seed mechanism has race conditions and doesn't cover graph mode (seeds SQLite but queries SurrealDB). Run the explicit db:seed script before E2E tests to ensure data exists for both architecture modes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove › symbol from field labels, align field text with node content (pl-1) - Wider tree line hover area (20px invisible wrapper) for easier collapse clicks - Filled background for collapsed bullet instead of outline ring - Dashed circle bullet for referenced nodes (same size as collapsed halo) - Add # icon to all supertag badges (node content, detail header, field refs) - Detail view: tags below header, radial gradient from last tag color - Hash icon import added to node-content, outline-editor, field-value Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add review command
| case 'Delete': { | ||
| e.preventDefault() | ||
| const prevId = getPreviousVisibleNode(selectedNodeId) | ||
| const nextId = getNextVisibleNode(selectedNodeId) | ||
| deleteNode(selectedNodeId) | ||
| selectNode(nextId ?? prevId ?? null) | ||
| break |
There was a problem hiding this comment.
🟡 Delete in selection mode selects a descendant that was also recursively deleted
In the global keydown handler for Backspace/Delete in selection mode, getNextVisibleNode(selectedNodeId) and getPreviousVisibleNode(selectedNodeId) are computed before deleteNode runs. If the selected node has visible children (is expanded), getNextVisibleNode returns the first child. After deleteNode, that child is recursively deleted too, but selectNode(nextId) still selects it. This leaves selectedNodeId pointing to a non-existent node, breaking keyboard navigation (arrow keys compute null because the ID isn't in the visible list) until the user clicks to reset selection.
Example scenario
Given an expanded tree A > [A1, A2], B, C where A is selected:
getNextVisibleNode('A')returns'A1'deleteNode('A')recursively deletes A, A1, A2selectNode('A1')selects a deleted node- Keyboard navigation is broken — should have selected
'B'instead
| case 'Delete': { | |
| e.preventDefault() | |
| const prevId = getPreviousVisibleNode(selectedNodeId) | |
| const nextId = getNextVisibleNode(selectedNodeId) | |
| deleteNode(selectedNodeId) | |
| selectNode(nextId ?? prevId ?? null) | |
| break | |
| case 'Backspace': | |
| case 'Delete': { | |
| e.preventDefault() | |
| const prevId = getPreviousVisibleNode(selectedNodeId) | |
| deleteNode(selectedNodeId) | |
| const nextId = getNextVisibleNode(prevId ?? '') | |
| selectNode(prevId ?? nextId ?? null) | |
| break | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
nxus-editormini-app with gateway routing on port 3005brainandnotepadicon types toMiniAppinterfacecubetobrainoxlintas dev dependency with rootlintscriptChanges
apps/nxus-gateway/src/config/mini-apps.ts— new editor entry, expanded icon union type, fixed recall iconapps/nxus-gateway/vite.config.ts—/editorproxy route, improved proxy error responsepackage.json—dev:editorscript, updateddevparallel target, addedoxlintandlintscriptSummary by CodeRabbit
New Features
Platform & Routing
Tests
Documentation
Chores