feat(ui): add dialog for review ref, tag, add TOC,...#274
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR refactors authentication client initialization from eager to lazy-loaded configuration, consolidates environment setup with Authentik integration, enhances Meilisearch with formatted result highlighting, introduces workspace SSE events for real-time updates, adds interactive reference/tag previews and document viewer mode, enables orphan node filtering in graph views, and redesigns the landing page with updated branding. ChangesAuth, environment, search, events, and workspace features
Landing page redesign
Infrastructure, database, and build configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR expands the UI editing experience (reference/tag previews, improved search UX, revision history improvements, and a table of contents) while also introducing workspace SSE event plumbing and refactoring environment/config handling for auth + Meilisearch across the Next.js app.
Changes:
- Added UI features: reference/tag preview dialogs, TOC, richer search highlighting, revision modal UX updates, and workspace member search/add flow.
- Introduced workspace SSE events context/provider and hooked it into the tree to invalidate queries on updates.
- Refactored auth client initialization (configurable baseURL) and moved server env typing/config into the web app; updated routing/env usage accordingly.
Reviewed changes
Copilot reviewed 58 out of 61 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui/src/lib/get-access-token-client-side.ts | Switches to getAuthClient() access token retrieval. |
| packages/ui/src/lib/auth-client.ts | Refactors auth client into configurable singleton (configureAuthClient/getAuthClient). |
| packages/ui/src/index.ts | Exports the new workspace events context. |
| packages/ui/src/hooks/use-search-cache.ts | Refactors debounce/caching behavior for search. |
| packages/ui/src/hooks/use-is-doc-modified.ts | Updates Yjs metadata map typing/keys and removes setter API. |
| packages/ui/src/hooks/use-editor-state.ts | Adjusts editor state usage to new useIsDocModified return shape. |
| packages/ui/src/hooks/use-debounced-value.ts | Adds a reusable debounced-value hook. |
| packages/ui/src/global.d.ts | Removes UI package env declarations. |
| packages/ui/src/contexts/workspace-events-context.tsx | Adds SSE workspace events provider + subscription API. |
| packages/ui/src/components/workspace-switcher.tsx | Formatting + minor structural adjustments (error UI moved). |
| packages/ui/src/components/workspace-sidebar.tsx | Uses getAuthClient() and updates projects navigation + logout handling. |
| packages/ui/src/components/workspace-members-modal.tsx | Adds server-side user search + “add member” flow with debounced search. |
| packages/ui/src/components/workspace-content-wrapper.tsx | Plumbs Meilisearch host via prop and wraps children in workspace events provider. |
| packages/ui/src/components/tree-view.tsx | Consumes workspace events to invalidate tree query; adjusts navigation handling. |
| packages/ui/src/components/trashed-file-management.tsx | Refactors styling and minor structural cleanup. |
| packages/ui/src/components/table-of-contents.tsx | Adds TOC popover UI driven by BlockNote headings + scroll progress. |
| packages/ui/src/components/signin-form.tsx | Uses getAuthClient() for social sign-in. |
| packages/ui/src/components/shadcn/popover.tsx | Adds shadcn popover primitive wrapper. |
| packages/ui/src/components/revision-modal.tsx | Improves revision modal UX (debounced search, confirm apply, better error handling). |
| packages/ui/src/components/note-title.tsx | Invalidates workspace tree after note rename. |
| packages/ui/src/components/note-suggestion-menu.tsx | Adds custom suggestion rendering for note mentions. |
| packages/ui/src/components/note-search-modal.tsx | Adds highlighted matches and safer search behavior when Meilisearch client is missing. |
| packages/ui/src/components/landing-navigation-bar.tsx | Updates marketing nav layout and link/button structure. |
| packages/ui/src/components/landing-hero.tsx | Replaces hero section with new marketing design. |
| packages/ui/src/components/landing-footer.tsx | Renames and redesigns marketing footer. |
| packages/ui/src/components/highlighted-text.tsx | Adds shared component to render Meilisearch-highlighted strings. |
| packages/ui/src/components/graph-view.tsx | Adds orphan filtering support to graph query + settings wiring. |
| packages/ui/src/components/graph-settings-dialog.tsx | Adds “Show Orphans” toggle (global graph only). |
| packages/ui/src/components/editor.tsx | Adds viewer-mode support, TOC rendering, and workspace role lookup. |
| packages/ui/src/components/editor-core.tsx | Adds viewer mode, note suggestion menu component, editor-ready callback. |
| packages/ui/src/block-note/tag.tsx | Adds tag click preview dialog with Meilisearch lookup + navigation. |
| packages/ui/src/block-note/reference.tsx | Adds reference click preview dialog with embedded read-only editor. |
| packages/ui/src/block-note/menu.ts | Adds Meilisearch highlight parsing and richer search result fields/snippets. |
| packages/ui/src/block-note/menu-states.ts | Makes menu state helper generic for richer suggestion items. |
| packages/ui/src/block-note/block-note.ts | Extends schema creation to accept API URL for reference previews. |
| packages/ui/oxlint.config.ts | Enables node plugin + disallows process.env usage in UI package. |
| nx.json | Adds run.configurations.mon. |
| internal/note/infra/persistence/pgsqlc/folder.sql.go | SQLC output updated: aliases folders AS f in recursive query. |
| internal/note/infra/persistence/pgsqlc/folder.sql | Source SQL updated to match aliasing change. |
| apps/web/tsconfig.json | Adds @lib/* path alias and includes lib/**/*.ts. |
| apps/web/proxy.ts | Switches auth import to local ./lib/auth. |
| apps/web/project.json | Adds start.configurations.mon. |
| apps/web/lib/get-access-token.ts | Switches auth import to local ./auth. |
| apps/web/lib/auth.ts | Switches access token import to local helper. |
| apps/web/global.d.ts | Adds NodeJS ProcessEnv typing for web app. |
| apps/web/Dockerfile | Adjusts standalone/static/public copy layout for production image. |
| apps/web/app/layout.tsx | Wraps app in EnvInit to configure auth client baseURL. |
| apps/web/app/env-init.tsx | Adds client-side auth client configuration initializer. |
| apps/web/app/api/auth/logout/route.ts | Adds Authentik end-session redirect endpoint. |
| apps/web/app/api/auth/[...all]/route.ts | Updates auth import alias. |
| apps/web/app/(workspace)/workspace/page.tsx | Updates access token helper import (@lib). |
| apps/web/app/(workspace)/workspace/[workspaceId]/page.tsx | Updates access token helper import (@lib). |
| apps/web/app/(workspace)/workspace/[workspaceId]/note/[noteId]/page.tsx | Uses API_URL for WS URL. |
| apps/web/app/(workspace)/workspace/[workspaceId]/layout.tsx | Passes Meilisearch host into UI wrapper. |
| apps/web/app/(workspace)/workspace/[workspaceId]/graph/page.tsx | Updates access token helper import (@lib). |
| apps/web/app/(marketing)/page.tsx | Simplifies marketing page wrapper. |
| apps/web/app/(marketing)/layout.tsx | Updates marketing layout spacing/container usage. |
| apps/web/.env | Adds server-side API_URL/MEILISEARCH_HOST and keeps other env values. |
| apps/document/src/hocuspocus/hocuspocus.ts | Adds onLoadDocument debug logging for document metadata. |
| apps/document/.env | Fixes NODE_ENV key name. |
| .dockerignore | Allows .next/standalone/node_modules to be included. |
Files not reviewed (1)
- internal/note/infra/persistence/pgsqlc/folder.sql.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
49
to
55
| return new Promise((resolve) => { | ||
| timeoutRef.current = setTimeout(async () => { | ||
| try { | ||
| const result = await searchFn(query); | ||
| setIsLoading(false); | ||
| setError(null); | ||
| resolve({ data: result, isLoading: false, error: null }); | ||
| } catch (err) { | ||
| const error = err instanceof Error ? err : new Error('Unknown error occurred'); | ||
| setError(error); | ||
| setIsLoading(false); | ||
| resolve({ data: undefined as T, isLoading: false, error }); | ||
| } | ||
| if (inflightQueryRef.current !== query) return; | ||
|
|
||
| const result = await doFetch(query); | ||
| resolve(result); | ||
| }, debounceMs); |
Comment on lines
+16
to
20
| const observer = (event: Y.YMapEvent<YDocMetadata>) => { | ||
| if (event.keysChanged.has('metadata')) { | ||
| setIsModified(metadata?.modified === true); | ||
| } | ||
| }; |
| if (isError) { | ||
| return ( | ||
| <div className="flex h-[400px] items-center justify-center"> | ||
| <div className="flex h-100 items-center justify-center"> |
|
|
||
| {!isCollapsed && ( | ||
| <ScrollArea className="h-[400px] pr-4"> | ||
| <ScrollArea className="h-100 pr-4"> |
| className="flex max-h-[80vh] max-w-4xl flex-col gap-0 overflow-hidden p-0" | ||
| showCloseButton={true} | ||
| > | ||
| <div className="min-h-75 flex-1 overflow-auto"> |
Comment on lines
+21
to
+25
| if (!meilisearchHost) { | ||
| console.warn( | ||
| 'MEILISEARCH_HOST is not set. Falling back to http://localhost:7700' | ||
| ); | ||
| } |
Comment on lines
+3
to
+12
| export function GET() { | ||
| const endSessionUrl = process.env.AUTHENTIK_CLIENT_DISCOVERY_URL?.replace( | ||
| '/.well-known/openid-configuration', | ||
| '/end-session/' | ||
| ); | ||
|
|
||
| const redirectUri = encodeURIComponent(`${process.env.BETTER_AUTH_URL}/signin`); | ||
|
|
||
| return NextResponse.redirect(`${endSessionUrl}?post_logout_redirect_uri=${redirectUri}`); | ||
| } |
Comment on lines
6
to
10
| NEXT_PUBLIC_API_URL="api.notopia.localhost" | ||
| API_URL="api.notopia.localhost" | ||
| AUTHENTIK_CLIENT_ID="app-web" | ||
| AUTHENTIK_CLIENT_SECRET="notopiauit" | ||
| AUTHENTIK_CLIENT_DISCOVERY_URL="http://authentik.notopia.localhost/application/o/app-web/.well-known/openid-configuration" |
Comment on lines
+96
to
+114
| <PopoverTrigger asChild> | ||
| <div | ||
| className="flex cursor-default flex-col items-center gap-4 px-1.5 py-3" | ||
| onMouseEnter={() => setOpen(true)} | ||
| onMouseLeave={() => setOpen(false)} | ||
| > | ||
| {Array.from({ length: SEGMENTS }).map((_, i) => ( | ||
| <div | ||
| key={i} | ||
| className={cn( | ||
| 'h-1 w-5 rounded-full transition-all duration-200', | ||
| i < filledSegments | ||
| ? 'bg-foreground/40 scale-115' | ||
| : 'bg-foreground/10 scale-100' | ||
| )} | ||
| /> | ||
| ))} | ||
| </div> | ||
| </PopoverTrigger> |
Comment on lines
+30
to
+36
| onLoadDocument: async (data) => { | ||
| this.logger.debug( | ||
| { documentId: data.documentName, documentMetadata: data.document.getMap('metadata') }, | ||
| 'Document loaded' | ||
| ); | ||
| await Promise.resolve(); | ||
| }, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/ui/src/components/note-title.tsx (1)
30-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways invalidate the note query after rename, regardless of
workspaceId.Current logic skips
getNoteOptionsinvalidation whenworkspaceIdis undefined, which can leave stale note title data in non-workspace contexts.Suggested fix
const { mutate: renameNote, isPending: isRenamingNote } = useRenameNoteMutation({ onSuccess: () => { - if (workspaceId) { - queryClient.invalidateQueries({ - queryKey: getNoteOptions({ path: { noteId } }).queryKey, - }); + queryClient.invalidateQueries({ + queryKey: getNoteOptions({ path: { noteId } }).queryKey, + }); + if (workspaceId) { queryClient.invalidateQueries({ queryKey: getWorkspaceTreeOptions({ path: { workspaceId } }).queryKey, }); } }, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/note-title.tsx` around lines 30 - 38, The onSuccess handler currently only invalidates getNoteOptions when workspaceId is truthy, leaving note title data stale in non-workspace contexts; change the logic in the onSuccess callback so queryClient.invalidateQueries({ queryKey: getNoteOptions({ path: { noteId } }).queryKey }) is always called, and keep the existing conditional around queryClient.invalidateQueries({ queryKey: getWorkspaceTreeOptions({ path: { workspaceId } }).queryKey }) so the workspace tree is only invalidated when workspaceId exists.apps/web/Dockerfile (1)
1-11:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop root privileges in the runtime image.
This stage still runs the app as
root. That unnecessarily widens the blast radius of any server-side compromise; add a dedicated runtime user and switch to it before the entrypoint.Proposed fix
FROM node:25.6.1-alpine3.23 AS production ENV NODE_ENV=production ENV NEXT_TELEMETRY_DISABLED=1 WORKDIR /app -COPY apps/web/.next/standalone ./ -COPY apps/web/public ./apps/web/public -COPY apps/web/.next/static ./apps/web/.next/static +RUN addgroup -S nodejs && adduser -S nextjs -G nodejs +COPY --chown=nextjs:nodejs apps/web/.next/standalone ./ +COPY --chown=nextjs:nodejs apps/web/public ./apps/web/public +COPY --chown=nextjs:nodejs apps/web/.next/static ./apps/web/.next/static +USER nextjs ENV PORT=3000 ENV HOSTNAME="0.0.0.0" EXPOSE 3000 ENTRYPOINT ["node", "apps/web/server.js"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/Dockerfile` around lines 1 - 11, The runtime image currently runs as root; create a non-root user and switch to it before starting the app by adding a dedicated runtime user (e.g., "appuser") and group, change ownership of the application files under WORKDIR to that user, and set USER to that username so ENTRYPOINT ["node","apps/web/server.js"] runs unprivileged; update the Dockerfile around WORKDIR/COPY steps and before ENTRYPOINT to add the user/group, chown the copied paths (./apps/web/* and ./.next/*) to that user, and set USER appuser.packages/ui/src/components/workspace-content-wrapper.tsx (1)
21-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not fall back to
localhostin the browser.When
meilisearchHostis missing, this silently points users at their own machine and turns a configuration error into opaque network failures. Disable search features or render an explicit misconfiguration state instead of defaulting tohttp://localhost:7700.Suggested fix
- if (!meilisearchHost) { - console.warn( - 'MEILISEARCH_HOST is not set. Falling back to http://localhost:7700' - ); - } + if (!meilisearchHost) { + console.warn('MEILISEARCH_HOST is not set. Search features will be disabled.'); + } @@ - <MeilisearchProvider - host={meilisearchHost || 'http://localhost:7700'} - apiKey={tokenData?.token} - > - <WorkspaceEventsProvider workspaceId={workspaceId}> - {children} - </WorkspaceEventsProvider> - <NoteSearchModal workspaceId={workspaceId} /> - </MeilisearchProvider> + <WorkspaceEventsProvider workspaceId={workspaceId}> + {meilisearchHost ? ( + <MeilisearchProvider host={meilisearchHost} apiKey={tokenData?.token}> + {children} + <NoteSearchModal workspaceId={workspaceId} /> + </MeilisearchProvider> + ) : ( + children + )} + </WorkspaceEventsProvider>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/workspace-content-wrapper.tsx` around lines 21 - 37, The current code silently falls back to 'http://localhost:7700' when meilisearchHost is missing; update the component (references: meilisearchHost, MeilisearchProvider, useQuery/getWorkspaceSearchTokenOptions, tokenData) to NOT default to localhost — instead, detect when meilisearchHost is falsy and render a clear misconfiguration state or disable search UI (e.g., do not render MeilisearchProvider and show a warning/error component), and ensure apiKey/token handling still guards against undefined tokenData; remove the fallback value and replace with a conditional render that surfaces the configuration error to the user or disables search features.packages/ui/src/components/editor.tsx (1)
40-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefault unknown workspace roles to read-only.
Whilst
getMyWorkspacesOptions()is still loading,currentWorkspaceisundefined, soisViewerbecomesfalseand viewer members get an editable editor on first render. Treat an unknown membership/role as read-only, or delay renderingEditorCoreuntil the query resolves.Suggested fix
const { data: allWorkspaceData } = useQuery({ ...getMyWorkspacesOptions({}), }); - const currentWorkspace = allWorkspaceData?.find((ws) => ws.workspace.id === workspaceId); - const isViewer = currentWorkspace?.role === 'viewer'; + const currentWorkspace = workspaceId + ? allWorkspaceData?.find((ws) => ws.workspace.id === workspaceId) + : undefined; + const isViewer = + workspaceId != null && + (!currentWorkspace || + currentWorkspace.role === 'viewer');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/editor.tsx` around lines 40 - 68, When workspace membership is unknown during the query, default to read-only to avoid briefly showing an editable editor: change the isViewer logic so unknown membership yields read-only (e.g., const isViewer = allWorkspaceData ? currentWorkspace?.role === 'viewer' : true) and additionally guard rendering EditorCore until the query resolves (only render EditorCore when allWorkspaceData is defined) so the editor isn't mounted with incorrect permissions; update references in this file (getMyWorkspacesOptions, currentWorkspace, isViewer, EditorCore, editorRef) accordingly.packages/ui/src/components/editor-core.tsx (1)
156-168:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix forwardRef ref handling to support callback refs
In
packages/ui/src/components/editor-core.tsx(lines 156-168), the effect castsreftoReact.MutableRefObjectand only sets.current; if a consumer passes a callback ref, it will never be called (and cleanup won’t clear it via the callback), breaking theforwardRefref contract.Suggested fix
-import { forwardRef, useMemo, useCallback, useEffect } from 'react'; +import { forwardRef, useMemo, useCallback, useEffect, useImperativeHandle } from 'react'; @@ - useEffect(() => { - if (ref) { - (ref as React.MutableRefObject<any>).current = editor; - } - if (onEditorReady) { - onEditorReady(editor); - } - return () => { - if (ref) { - (ref as React.MutableRefObject<any>).current = null; - } - }; - }, [editor, ref, onEditorReady]); + useImperativeHandle(ref, () => editor, [editor]); + + useEffect(() => { + onEditorReady?.(editor); + }, [editor, onEditorReady]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/editor-core.tsx` around lines 156 - 168, The effect currently assumes ref is a MutableRefObject and only sets ref.current; change it to handle both object refs and callback refs: inside the useEffect that references editor, ref, and onEditorReady, detect if typeof ref === 'function' and call ref(editor) (and on cleanup call ref(null)), otherwise cast to React.MutableRefObject and set .current = editor (and .current = null in cleanup); keep calling onEditorReady(editor) as before. This preserves forwardRef callback semantics and proper cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.dockerignore:
- Around line 2-3: The current .dockerignore negation only matches the directory
path and not its contents, so the standalone bundle's dependencies remain
excluded; update the negation to re-include the subtree by changing
"!**/.next/standalone/node_modules" to include contents (for example
"!**/.next/standalone/node_modules/**" and optionally also
"!**/.next/standalone/node_modules") while keeping the general exclusion
"**/node_modules".
In `@apps/document/src/hocuspocus/hocuspocus.ts`:
- Around line 30-33: The onLoadDocument handler is currently logging the full
Yjs metadata map (data.document.getMap('metadata')), which may expose sensitive
or oversized data; update onLoadDocument to extract and log only bounded
non-sensitive fields (e.g. const meta = data.document.getMap('metadata'); const
modified = meta.get('modified'); const keyCount = meta.size or
meta.keys().length) and pass those values to this.logger.debug along with
documentId instead of the entire map; locate the onLoadDocument callback and
replace the logger.debug payload accordingly so only safe, small fields are
logged.
In `@apps/web/app/`(workspace)/workspace/[workspaceId]/note/[noteId]/page.tsx:
- Line 15: The WebSocket URL construction using
url={`ws://${process.env.API_URL}/document/ws/document`} is protocol‑fragile;
change it to build the ws/wss scheme dynamically from process.env.API_URL (or
parse it with the URL API) so if API_URL already contains a scheme you convert
http->ws and https->wss (or strip any existing scheme and prepend the correct
ws/wss), and ensure the final value passed to the url prop is a valid ws:// or
wss:// URL.
In `@apps/web/app/api/auth/logout/route.ts`:
- Around line 3-11: The GET handler builds endSessionUrl and redirectUri from
envs without validation; update the GET function to validate that
process.env.AUTHENTIK_CLIENT_DISCOVERY_URL and process.env.BETTER_AUTH_URL are
present and that AUTHENTIK_CLIENT_DISCOVERY_URL contains
'/.well-known/openid-configuration' before calling replace; if validation fails,
return a controlled error response (e.g., NextResponse.json or
NextResponse.redirect to a safe error page) with an appropriate 4xx/5xx status
instead of redirecting to an invalid URL; ensure you reference and check the
endSessionUrl and redirectUri construction paths so the logic short-circuits on
missing/invalid envs.
In `@apps/web/app/env-init.tsx`:
- Around line 14-22: EnvInit currently calls configureAuthClient(betterAuthUrl
|| 'http://localhost:3000') during render guarded by a module-level configured
flag; move that side-effect into a React useEffect inside the EnvInit component
(remove the module-level configured mutable variable) so configuration runs only
after mount and is safe under StrictMode. Inside the effect, preserve the
existing warning when betterAuthUrl is falsy and call
configureAuthClient(betterAuthUrl || 'http://localhost:3000'); use an empty
dependency array (or a stable ref) to ensure it only runs once.
In `@apps/web/app/layout.tsx`:
- Around line 46-48: The layout currently passes a fallback
"http://localhost:3000" to EnvInit which can hide misconfigurations; instead,
read process.env.BETTER_AUTH_URL and fail fast if it's undefined or empty, then
pass that value into EnvInit. Update the code around EnvInit so it does not
provide a localhost default — validate BETTER_AUTH_URL (e.g., throw or assert
with a clear message mentioning BETTER_AUTH_URL) before rendering and then
supply the validated string to the EnvInit betterAuthUrl prop.
In `@packages/ui/src/block-note/menu.ts`:
- Around line 123-125: The Meilisearch filter is built by interpolating raw tag
text into `tags = "${tag}"` in the index.search call, which breaks or allows
injection when tag contains `"` or `\`; fix it by escaping backslashes and
double quotes in the tag before interpolation (or otherwise using a safe filter
construction API) so the filter string is always valid; update the code around
the `index.search('', { filter: [\`tags = "${tag}"\`], limit: 50 })` call to
sanitize `tag` (escape `\` and `"` characters) or use a safe filter-building
helper, ensuring the search uses the escaped value.
In `@packages/ui/src/block-note/reference.tsx`:
- Around line 88-95: The anchor's fallback href uses `/note/${noteId}` but the
real route is nested under the workspace path, so update the element that
renders the preview link: either construct the correct absolute route (e.g.,
`/workspace/${workspaceId}/note/${noteId}`) and use that as the href, or replace
the <a> with a semantic <button> (preserving className, data-notopia-ref and the
onClick that calls setShowPreview) to avoid misleading open-in-new-tab/copy-link
behavior; locate the anchor using the attributes noteId, href, onClick and
setShowPreview and make the change accordingly so keyboard/accessibility
(role/aria) is preserved.
- Around line 109-115: The preview socket URL construction in
createBlockNoteReferenceSpec currently hardcodes "ws://", which breaks when
apiUrl includes a scheme or when the page is served over HTTPS; update the
previewWsUrl logic to derive the WebSocket scheme and host from apiUrl (or from
window.location if apiUrl is relative/missing): if apiUrl starts with "http://"
or "https://", map "http"->"ws" and "https"->"wss" and replace the scheme before
appending "/document/ws/document"; if apiUrl has no scheme, use
window.location.protocol to choose "ws" vs "wss" and build the full URL
accordingly so previewWsUrl always uses the correct ws/wss scheme.
In `@packages/ui/src/components/graph-settings-dialog.tsx`:
- Line 335: The onCheckedChange handlers in graph-settings-dialog.tsx (e.g., the
one calling onOrphansChange) unsafely cast Radix CheckedState to boolean; change
each handler to pass a strict boolean by using a strict equality check (e.g.,
checked === true) instead of casting, update all similar checkbox handlers in
this file (those that call onOrphansChange, and other onXChange callbacks) to
use checked === true so 'indeterminate' is handled safely and only true becomes
true.
In `@packages/ui/src/components/landing-footer.tsx`:
- Around line 17-21: Replace the non-interactive <span> elements that render
"Blog", "Docs", and "Source" in the landing-footer component with real
interactive link elements so they are keyboard-accessible and navigable: locate
the three spans (text nodes "Blog", "Docs", "Source") and change them to <a> (or
your framework's Link) elements with appropriate hrefs (e.g., blog URL, docs
route, repository URL), keep the existing className ("hover:text-foreground
cursor-pointer transition-colors") for styling, and ensure external links
include target="_blank" rel="noopener noreferrer" where applicable; leave the
Separator component usage unchanged.
In `@packages/ui/src/components/landing-hero.tsx`:
- Around line 34-37: The contact input currently uses only placeholder text
("Contact us") which is not a reliable accessible name; update the Input element
(the Input component instance with placeholder "Contact us") to provide an
explicit accessible name by adding an aria-label (e.g., aria-label="Contact us")
or by associating it with a visible or visually-hidden <label> (generate an id
on the Input and reference it from the label) so assistive technologies get a
proper name.
In `@packages/ui/src/components/landing-navigation-bar.tsx`:
- Around line 28-33: The menu items are rendered as interactive controls but
lack navigation targets; update NAV_ITEMS to be an array of objects like {label,
href} (or add hrefs where NAV_ITEMS is defined), then replace the current
NavigationMenuTrigger usage inside the NAV_ITEMS.map with a link-capable element
(e.g., NavigationMenuLink or your app's Link component) that uses the item's
href so clicking the NavigationMenuItem navigates; also wire the GitHub CTA (the
button rendered at the other block) to a proper anchor or Link with href,
target="_blank" and rel="noopener noreferrer" so it actually opens the GitHub
destination.
In `@packages/ui/src/components/revision-modal.tsx`:
- Around line 133-139: handleOpenChange currently resets setSelectedRevisionId
and setSearchQuery when the modal closes but doesn't clear the confirmApply
flag; update the callback (handleOpenChange) so that when newOpen is false you
also call setConfirmApply(false) to reset the confirmation dialog state
(alongside setSelectedRevisionId(null) and setSearchQuery('')) so confirmApply
cannot remain stale after the main dialog closes.
- Around line 44-49: resolvedTheme from useTheme() can be undefined and the
current type assertion hides that risk; in revision-modal.tsx compute a safe
theme value (e.g., const theme = resolvedTheme ?? 'light' or normalize with
(resolvedTheme === 'dark' ? 'dark' : 'light')) and pass that to BlockNoteView
instead of casting resolvedTheme, updating the code where useTheme() is called
and where BlockNoteView is rendered to use the new theme variable.
In `@packages/ui/src/components/shadcn/popover.tsx`:
- Around line 58-64: PopoverTitle is typed with React.ComponentProps<"h2"> but
returns a div, breaking semantics and accessibility; change the JSX element
returned by the PopoverTitle function from <div> to an <h2> while keeping
data-slot="popover-title", the cn("font-medium", className) className logic and
spreading {...props} so the component's props and styling remain unchanged.
In `@packages/ui/src/components/table-of-contents.tsx`:
- Around line 95-122: The TOC popover is currently opened only via hover
handlers on a non-focusable div, preventing keyboard/touch access; replace the
PopoverTrigger child div with a focusable shadcn Button (use the existing
Popover, PopoverTrigger and PopoverContent) and remove the
onMouseEnter/onMouseLeave handlers from both the trigger and PopoverContent so
Radix/Popover handles click and focus for keyboard/touch users; keep the visual
rendering using SEGMENTS and filledSegments and retain setOpen only if you still
need controlled open state, otherwise rely on Radix's default open behavior.
In `@packages/ui/src/components/trashed-file-management.tsx`:
- Around line 342-349: The two DropdownMenuItem entries are just UI-only; wire
them to the restore and permanent-delete mutations by adding onClick/onSelect
handlers that call the existing mutation functions (e.g.,
restoreFileMutation.mutate or deleteFileMutation.mutate) with the current file
identifier (e.g., file.id or trashedItem.id) and handle optimistic UI/update on
success; update the DropdownMenuItem for the RotateCcw item to call the restore
handler and the Trash2 item to call the delete-permanent handler, and ensure any
required imports (restore/delete mutation hooks) and loading/disabled states are
applied so the actions actually execute.
In `@packages/ui/src/components/workspace-members-modal.tsx`:
- Around line 107-118: The current useQuery call that sets const { data:
searchUserData = [], isFetching: isSearchingUsers } = useQuery({...}) masks
network/errors by defaulting data to []—capture and expose the query error state
by adding isError and error to the destructure (e.g. const { data:
searchUserData = [], isFetching: isSearchingUsers, isError:
isSearchingUsersError, error: searchUsersError } = useQuery({...})) for the
useQuery invoked with searchUsersOptions and select: mapDtoToSearchUserMembers,
then update the UI rendering where searchUserData/isSearchingUsers are used
(including the similar block around lines 237-249) to show an explicit
error/failure state or alert when isSearchingUsersError is true using
searchUsersError instead of showing the "No users found" empty state.
In `@packages/ui/src/components/workspace-sidebar.tsx`:
- Around line 440-444: The onClick handler's await getAuthClient().signOut() can
reject and currently prevents navigation to the provider logout, so wrap the
sign-out call in a try/catch or use try { await getAuthClient().signOut(); }
catch (err) { /* optional log via console or process logger */ } finally {
window.location.href = '/api/auth/logout'; } to ensure the DropdownMenuItem
click always navigates to '/api/auth/logout' even if getAuthClient().signOut()
fails.
In `@packages/ui/src/components/workspace-switcher.tsx`:
- Around line 324-343: The role selectors (Select using
value={editForm.userRole}, setEditForm and RoleSelectItems) are only updating UI
state and not persisted because the slug update and workspace creation mutations
do not include userRole in their payloads; either remove these Select controls
from the workspace-switcher UI or wire them to a real role-management mutation
by (a) adding userRole to the payload of the workspace creation and slug update
flows (ensure the handlers that call those mutations—where the createWorkspace
and updateWorkspaceSlug functions are invoked—read editForm.userRole and pass it
through), or (b) call a dedicated role-assignment API/mutation after save (e.g.,
assignRoleToUser(workspaceId, userId, editForm.userRole)), and ensure the
SelectTrigger onClick still stops propagation and the SelectValue reflects the
persisted value.
- Around line 127-131: The alert currently exposes raw backend error details by
interpolating error.message into showAlert; change the user-facing message to a
generic, non-sensitive string (e.g., "There was an error creating your
workspace. Please try again.") and remove interpolation of error.message from
the message passed to showAlert; instead send the full error to your
telemetry/logging helper (e.g., reportError, logError, or the existing telemetry
client) so detailed diagnostics are recorded. Apply the same fix for the other
occurrence around lines 185-189, and ensure you reference the same showAlert
call sites and pass the error object to your telemetry function rather than to
the visible alert.
In `@packages/ui/src/contexts/workspace-events-context.tsx`:
- Around line 38-40: The fan-out in onSseEvent uses
handlersRef.current.forEach((handler) => handler(raw.data)) so a throwing
handler aborts delivery; change this to call each handler inside its own
try/catch (e.g., iterate handlersRef.current and for each handler invoke
handler(raw.data) inside try { ... } catch (err) { /* log error */ }) so one
subscriber failure doesn't stop others, and log the error with context including
the handler identity or event data.
- Around line 33-55: The connect logic in connect/getWorkspaceEvents currently
stops on first error or when the stream closes; wrap the connection lifecycle in
a retry loop with exponential backoff so it reconnects until the AbortController
is signaled. Specifically, modify the connect function used in the
workspace-events effect to: (1) loop while !abortController.signal.aborted, (2)
call getWorkspaceEvents(...) and await/consume its stream inside a try block,
(3) on success reset the backoff, (4) on any thrown error or when the
for-await-of completes treat it as a disconnect and wait an increasing delay
(with a max cap) before retrying, and (5) ensure you still respect
onSseEvent/onSseError handlersRef and abortController.signal to break the loop
promptly; update functions named connect and the effect that calls it
accordingly.
In `@packages/ui/src/hooks/use-is-doc-modified.ts`:
- Line 27: The effect in the useIsDocModified hook currently includes clientId
in its dependency array though clientId is not referenced inside the effect
body; update the hook by removing clientId from the useEffect dependency array
(leaving [ydoc]) to avoid unnecessary re-runs, or if clientId is truly unused
anywhere in the hook, remove the clientId parameter from the useIsDocModified
signature and all callers instead; locate references to useIsDocModified and the
useEffect that ends with "}, [ydoc, clientId]);" and apply one of these two
fixes consistently.
- Around line 16-20: The observer callback in use-is-doc-modified captures a
stale metadata variable, so update the observer to read the current metadata
from the Y map at runtime (e.g., use event.target.get('metadata') or fetch from
the Y doc/map inside the observer) instead of referencing the closed-over
metadata; then call setIsModified(currentMetadata?.modified === true) so
isModified reflects real-time changes (ensure the observer signature remains
Y.YMapEvent<YDocMetadata> and is registered/unregistered the same way).
In `@packages/ui/src/hooks/use-search-cache.ts`:
- Around line 49-56: The returned Promise in the debounce block (inside
use-search-cache's search logic) can remain unresolved if
inflightQueryRef.current !== query; modify that early-return to settle the
Promise by either resolving with a defined "stale" result (e.g., null or an
empty SearchResult) or rejecting with a specific CancelledError so callers can
distinguish cancellation from real failures; ensure you also clear
timeoutRef.current and do any necessary cleanup before resolving/rejecting, and
update callers of search() to handle the chosen sentinel or error type.
In `@packages/ui/src/lib/auth-client.ts`:
- Around line 7-20: getAuthClient currently silently defaults to a localhost
base URL when configureAuthClient hasn't been called; change getAuthClient to
fail fast by checking _baseURL (and treating empty string as unset) and throw a
clear error instructing callers to call configureAuthClient(baseURL) first; keep
the existing createAuthClient usage and _client caching behavior (resetting
_client in configureAuthClient remains fine) so the only change is adding a
guard at the start of getAuthClient that throws a descriptive Error if _baseURL
is falsy.
---
Outside diff comments:
In `@apps/web/Dockerfile`:
- Around line 1-11: The runtime image currently runs as root; create a non-root
user and switch to it before starting the app by adding a dedicated runtime user
(e.g., "appuser") and group, change ownership of the application files under
WORKDIR to that user, and set USER to that username so ENTRYPOINT
["node","apps/web/server.js"] runs unprivileged; update the Dockerfile around
WORKDIR/COPY steps and before ENTRYPOINT to add the user/group, chown the copied
paths (./apps/web/* and ./.next/*) to that user, and set USER appuser.
In `@packages/ui/src/components/editor-core.tsx`:
- Around line 156-168: The effect currently assumes ref is a MutableRefObject
and only sets ref.current; change it to handle both object refs and callback
refs: inside the useEffect that references editor, ref, and onEditorReady,
detect if typeof ref === 'function' and call ref(editor) (and on cleanup call
ref(null)), otherwise cast to React.MutableRefObject and set .current = editor
(and .current = null in cleanup); keep calling onEditorReady(editor) as before.
This preserves forwardRef callback semantics and proper cleanup.
In `@packages/ui/src/components/editor.tsx`:
- Around line 40-68: When workspace membership is unknown during the query,
default to read-only to avoid briefly showing an editable editor: change the
isViewer logic so unknown membership yields read-only (e.g., const isViewer =
allWorkspaceData ? currentWorkspace?.role === 'viewer' : true) and additionally
guard rendering EditorCore until the query resolves (only render EditorCore when
allWorkspaceData is defined) so the editor isn't mounted with incorrect
permissions; update references in this file (getMyWorkspacesOptions,
currentWorkspace, isViewer, EditorCore, editorRef) accordingly.
In `@packages/ui/src/components/note-title.tsx`:
- Around line 30-38: The onSuccess handler currently only invalidates
getNoteOptions when workspaceId is truthy, leaving note title data stale in
non-workspace contexts; change the logic in the onSuccess callback so
queryClient.invalidateQueries({ queryKey: getNoteOptions({ path: { noteId }
}).queryKey }) is always called, and keep the existing conditional around
queryClient.invalidateQueries({ queryKey: getWorkspaceTreeOptions({ path: {
workspaceId } }).queryKey }) so the workspace tree is only invalidated when
workspaceId exists.
In `@packages/ui/src/components/workspace-content-wrapper.tsx`:
- Around line 21-37: The current code silently falls back to
'http://localhost:7700' when meilisearchHost is missing; update the component
(references: meilisearchHost, MeilisearchProvider,
useQuery/getWorkspaceSearchTokenOptions, tokenData) to NOT default to localhost
— instead, detect when meilisearchHost is falsy and render a clear
misconfiguration state or disable search UI (e.g., do not render
MeilisearchProvider and show a warning/error component), and ensure apiKey/token
handling still guards against undefined tokenData; remove the fallback value and
replace with a conditional render that surfaces the configuration error to the
user or disables search features.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a736007-5def-4aec-a026-14be53563316
📒 Files selected for processing (61)
.dockerignoreapps/document/.envapps/document/src/hocuspocus/hocuspocus.tsapps/web/.envapps/web/Dockerfileapps/web/app/(marketing)/layout.tsxapps/web/app/(marketing)/page.tsxapps/web/app/(workspace)/workspace/[workspaceId]/graph/page.tsxapps/web/app/(workspace)/workspace/[workspaceId]/layout.tsxapps/web/app/(workspace)/workspace/[workspaceId]/note/[noteId]/page.tsxapps/web/app/(workspace)/workspace/[workspaceId]/page.tsxapps/web/app/(workspace)/workspace/page.tsxapps/web/app/api/auth/[...all]/route.tsapps/web/app/api/auth/logout/route.tsapps/web/app/env-init.tsxapps/web/app/layout.tsxapps/web/global.d.tsapps/web/lib/auth.tsapps/web/lib/get-access-token.tsapps/web/project.jsonapps/web/proxy.tsapps/web/tsconfig.jsoninternal/note/infra/persistence/pgsqlc/folder.sqlinternal/note/infra/persistence/pgsqlc/folder.sql.gonx.jsonpackages/ui/oxlint.config.tspackages/ui/src/block-note/block-note.tspackages/ui/src/block-note/menu-states.tspackages/ui/src/block-note/menu.tspackages/ui/src/block-note/reference.tsxpackages/ui/src/block-note/tag.tsxpackages/ui/src/components/editor-core.tsxpackages/ui/src/components/editor.tsxpackages/ui/src/components/graph-settings-dialog.tsxpackages/ui/src/components/graph-view.tsxpackages/ui/src/components/highlighted-text.tsxpackages/ui/src/components/landing-footer.tsxpackages/ui/src/components/landing-hero.tsxpackages/ui/src/components/landing-navigation-bar.tsxpackages/ui/src/components/note-search-modal.tsxpackages/ui/src/components/note-suggestion-menu.tsxpackages/ui/src/components/note-title.tsxpackages/ui/src/components/revision-modal.tsxpackages/ui/src/components/shadcn/popover.tsxpackages/ui/src/components/signin-form.tsxpackages/ui/src/components/table-of-contents.tsxpackages/ui/src/components/trashed-file-management.tsxpackages/ui/src/components/tree-view.tsxpackages/ui/src/components/workspace-content-wrapper.tsxpackages/ui/src/components/workspace-members-modal.tsxpackages/ui/src/components/workspace-sidebar.tsxpackages/ui/src/components/workspace-switcher.tsxpackages/ui/src/contexts/workspace-events-context.tsxpackages/ui/src/global.d.tspackages/ui/src/hooks/use-debounced-value.tspackages/ui/src/hooks/use-editor-state.tspackages/ui/src/hooks/use-is-doc-modified.tspackages/ui/src/hooks/use-search-cache.tspackages/ui/src/index.tspackages/ui/src/lib/auth-client.tspackages/ui/src/lib/get-access-token-client-side.ts
💤 Files with no reviewable changes (1)
- packages/ui/src/global.d.ts
Comment on lines
2
to
+3
| **/node_modules | ||
| !**/.next/standalone/node_modules |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Docker .dockerignoresyntax, if/node_modulesis ignored and!/.next/standalone/node_modulesis added, are files under that directory re-included automatically, or do you also need!/.next/standalone/node_modules/?
💡 Result:
You need the second rule. In .dockerignore, a negation rule like !/.next/standalone/node_modules only re-includes what it explicitly matches (that exact directory path). It does not automatically re-include the directory’s contents; those subpaths can still be excluded by the earlier /node_modules pattern. To re-include the contents, add:!/.next/standalone/node_modules/ Why: .dockerignore treats ! lines as exceptions to prior exclusion patterns, and the Docker docs/behavior follow “last matching line wins” semantics for inclusion/exclusion per path [1]. Additionally, it’s not possible to re-include a file if any parent directory of that file is excluded [2], so you must ensure the matching rule covers the subpaths you want included. So: !/.next/standalone/node_modules is not sufficient by itself; include / as well.
Citations:
- 1: https://github.com/docker/docker-ce/blob/master/components/cli/docs/reference/builder.md
- 2: Verify parity between .dockerignore and .gitignore syntax moby/moby#40318
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la .dockerignore
nl -ba .dockerignore | sed -n '1,120p'Repository: notopia-uit/notopia
Length of output: 186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n .dockerignoreRepository: notopia-uit/notopia
Length of output: 1079
🌐 Web query:
In Docker .dockerignore, does a negation rule like !path/to/dirre-include the directory’s contents, or do you need!path/to/dir/**(or!/dir/) to include child files?
💡 Result:
In a.dockerignore file, a simple negation rule like!path/to/dir is generally insufficient to include the directory's contents if the directory itself or its parent paths have been excluded [1][2]. Because Docker's exclusion logic mirrors behavior where parent directories are often skipped entirely for performance reasons, simply negating the directory name does not automatically re-include the files contained within it [3][4][2]. To ensure a directory and all its recursive contents are included, you typically need to use a pattern that matches the directory's contents explicitly, such as!path/to/dir/ [1][5]. If you only use!path/to/dir, Docker may treat that as a command to include the directory entry itself, but it may still refuse to descend into that directory to pick up the files within it because the path was already excluded [1][2]. Using!path/to/dir/ ensures that the pattern matches the directory and all of its sub-contents, effectively overriding previous exclusion rules for the entire tree [5]. Top results: [1][2][5]
Citations:
- 1: dockerignore: Exclamation mark with the pattern ** isn't including all sub directories's files but only the top level one docker/cli#2919
- 2: Verify parity between .dockerignore and .gitignore syntax moby/moby#40318
- 3: https://mirrors.edge.kernel.org/pub/software/scm/git/docs/gitignore.html
- 4: https://git-scm.com/docs/gitignore
- 5: https://medium.com/@LihauTan/took-me-hours-to-realise-why-docker-build-ignores-my-dockerignore-and-this-is-what-ive-learned-2f87c770ea9c
Re-include the standalone node_modules contents in Docker build context
In .dockerignore, line 3 (!**/.next/standalone/node_modules) only negates the directory path, while line 2 (**/node_modules) still excludes the subtree contents—so the standalone bundle may miss its packaged dependencies.
**/node_modules
!**/.next/standalone/node_modules
Proposed fix
**/node_modules
!**/.next/standalone/node_modules
+!**/.next/standalone/node_modules/**📝 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.
Suggested change
| **/node_modules | |
| !**/.next/standalone/node_modules | |
| **/node_modules | |
| !**/.next/standalone/node_modules | |
| !**/.next/standalone/node_modules/** |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.dockerignore around lines 2 - 3, The current .dockerignore negation only
matches the directory path and not its contents, so the standalone bundle's
dependencies remain excluded; update the negation to re-include the subtree by
changing "!**/.next/standalone/node_modules" to include contents (for example
"!**/.next/standalone/node_modules/**" and optionally also
"!**/.next/standalone/node_modules") while keeping the general exclusion
"**/node_modules".
Comment on lines
+30
to
+33
| onLoadDocument: async (data) => { | ||
| this.logger.debug( | ||
| { documentId: data.documentName, documentMetadata: data.document.getMap('metadata') }, | ||
| 'Document loaded' |
Contributor
There was a problem hiding this comment.
Avoid logging the full document metadata map.
Logging data.document.getMap('metadata') can leak internal/sensitive metadata and produce oversized logs. Please log only bounded, non-sensitive fields (for example, metadata.modified or key count).
Proposed fix
onLoadDocument: async (data) => {
+ const metadata = data.document.getMap('metadata') as Y.Map<unknown>;
+ const metadataEntry = metadata.get('metadata') as { modified?: boolean } | undefined;
this.logger.debug(
- { documentId: data.documentName, documentMetadata: data.document.getMap('metadata') },
+ {
+ documentId: data.documentName,
+ isModified: Boolean(metadataEntry?.modified),
+ metadataKeysCount: metadata.size,
+ },
'Document loaded'
);
await Promise.resolve();
},📝 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.
Suggested change
| onLoadDocument: async (data) => { | |
| this.logger.debug( | |
| { documentId: data.documentName, documentMetadata: data.document.getMap('metadata') }, | |
| 'Document loaded' | |
| onLoadDocument: async (data) => { | |
| const metadata = data.document.getMap('metadata') as Y.Map<unknown>; | |
| const metadataEntry = metadata.get('metadata') as { modified?: boolean } | undefined; | |
| this.logger.debug( | |
| { | |
| documentId: data.documentName, | |
| isModified: Boolean(metadataEntry?.modified), | |
| metadataKeysCount: metadata.size, | |
| }, | |
| 'Document loaded' | |
| ); | |
| await Promise.resolve(); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/document/src/hocuspocus/hocuspocus.ts` around lines 30 - 33, The
onLoadDocument handler is currently logging the full Yjs metadata map
(data.document.getMap('metadata')), which may expose sensitive or oversized
data; update onLoadDocument to extract and log only bounded non-sensitive fields
(e.g. const meta = data.document.getMap('metadata'); const modified =
meta.get('modified'); const keyCount = meta.size or meta.keys().length) and pass
those values to this.logger.debug along with documentId instead of the entire
map; locate the onLoadDocument callback and replace the logger.debug payload
accordingly so only safe, small fields are logged.
| <div className="p-4"> | ||
| <HocuspocusProviderWebsocketComponent | ||
| url={`ws://${process.env.NEXT_PUBLIC_API_URL}/document/ws/document`} | ||
| url={`ws://${process.env.API_URL}/document/ws/document`} |
Contributor
There was a problem hiding this comment.
WebSocket endpoint construction is protocol-fragile and can produce invalid URLs.
On Line 15, prefixing with ws:// is unsafe: if process.env.API_URL already contains http(s)://, the result is malformed, and HTTPS deployments should use wss://.
Proposed fix
- url={`ws://${process.env.API_URL}/document/ws/document`}
+ url={(() => {
+ const api = new URL(process.env.API_URL as string);
+ const wsProtocol = api.protocol === 'https:' ? 'wss:' : 'ws:';
+ return `${wsProtocol}//${api.host}/document/ws/document`;
+ })()}📝 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.
Suggested change
| url={`ws://${process.env.API_URL}/document/ws/document`} | |
| url={(() => { | |
| const api = new URL(process.env.API_URL as string); | |
| const wsProtocol = api.protocol === 'https:' ? 'wss:' : 'ws:'; | |
| return `${wsProtocol}//${api.host}/document/ws/document`; | |
| })()} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/app/`(workspace)/workspace/[workspaceId]/note/[noteId]/page.tsx at
line 15, The WebSocket URL construction using
url={`ws://${process.env.API_URL}/document/ws/document`} is protocol‑fragile;
change it to build the ws/wss scheme dynamically from process.env.API_URL (or
parse it with the URL API) so if API_URL already contains a scheme you convert
http->ws and https->wss (or strip any existing scheme and prepend the correct
ws/wss), and ensure the final value passed to the url prop is a valid ws:// or
wss:// URL.
Comment on lines
+3
to
+11
| export function GET() { | ||
| const endSessionUrl = process.env.AUTHENTIK_CLIENT_DISCOVERY_URL?.replace( | ||
| '/.well-known/openid-configuration', | ||
| '/end-session/' | ||
| ); | ||
|
|
||
| const redirectUri = encodeURIComponent(`${process.env.BETTER_AUTH_URL}/signin`); | ||
|
|
||
| return NextResponse.redirect(`${endSessionUrl}?post_logout_redirect_uri=${redirectUri}`); |
Contributor
There was a problem hiding this comment.
Validate the logout URLs before redirecting.
If AUTHENTIK_CLIENT_DISCOVERY_URL or BETTER_AUTH_URL is missing, this emits a redirect with an invalid target instead of a controlled server error. The same applies if the discovery URL does not contain the expected suffix.
💡 Proposed fix
export function GET() {
- const endSessionUrl = process.env.AUTHENTIK_CLIENT_DISCOVERY_URL?.replace(
+ const discoveryUrl = process.env.AUTHENTIK_CLIENT_DISCOVERY_URL;
+ const betterAuthUrl = process.env.BETTER_AUTH_URL;
+
+ if (!discoveryUrl || !betterAuthUrl) {
+ return NextResponse.json({ message: 'Auth logout is not configured' }, { status: 500 });
+ }
+
+ const endSessionUrl = discoveryUrl.replace(
'/.well-known/openid-configuration',
'/end-session/'
);
-
- const redirectUri = encodeURIComponent(`${process.env.BETTER_AUTH_URL}/signin`);
+ if (endSessionUrl === discoveryUrl) {
+ return NextResponse.json({ message: 'Invalid discovery URL' }, { status: 500 });
+ }
+
+ const redirectUri = encodeURIComponent(`${betterAuthUrl}/signin`);
return NextResponse.redirect(`${endSessionUrl}?post_logout_redirect_uri=${redirectUri}`);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/app/api/auth/logout/route.ts` around lines 3 - 11, The GET handler
builds endSessionUrl and redirectUri from envs without validation; update the
GET function to validate that process.env.AUTHENTIK_CLIENT_DISCOVERY_URL and
process.env.BETTER_AUTH_URL are present and that AUTHENTIK_CLIENT_DISCOVERY_URL
contains '/.well-known/openid-configuration' before calling replace; if
validation fails, return a controlled error response (e.g., NextResponse.json or
NextResponse.redirect to a safe error page) with an appropriate 4xx/5xx status
instead of redirecting to an invalid URL; ensure you reference and check the
endSessionUrl and redirectUri construction paths so the logic short-circuits on
missing/invalid envs.
Comment on lines
+14
to
+22
| if (!configured) { | ||
| if (!betterAuthUrl) { | ||
| console.warn( | ||
| 'BETTER_AUTH_URL is not set. Falling back to http://localhost:3000' | ||
| ); | ||
| } | ||
| configureAuthClient(betterAuthUrl || 'http://localhost:3000'); | ||
| configured = true; | ||
| } |
Contributor
There was a problem hiding this comment.
Move auth client configuration out of EnvInit’s render path.
apps/web/app/env-init.tsx calls configureAuthClient(...) during render (guarded by a module-level configured flag). This makes render impure and can run unexpectedly under React StrictMode/concurrent rendering. Move the configuration into a useEffect (and avoid module-level mutable state) while keeping the existing BETTER_AUTH_URL warning + http://localhost:3000 fallback behaviour.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/app/env-init.tsx` around lines 14 - 22, EnvInit currently calls
configureAuthClient(betterAuthUrl || 'http://localhost:3000') during render
guarded by a module-level configured flag; move that side-effect into a React
useEffect inside the EnvInit component (remove the module-level configured
mutable variable) so configuration runs only after mount and is safe under
StrictMode. Inside the effect, preserve the existing warning when betterAuthUrl
is falsy and call configureAuthClient(betterAuthUrl || 'http://localhost:3000');
use an empty dependency array (or a stable ref) to ensure it only runs once.
Comment on lines
+38
to
+40
| onSseEvent: (raw) => { | ||
| handlersRef.current.forEach((handler) => handler(raw.data)); | ||
| }, |
Contributor
There was a problem hiding this comment.
Isolate subscriber failures during fan-out.
One throwing handler currently aborts delivery to the remaining subscribers and can bubble into the SSE path. Wrap each callback individually so a single broken consumer does not take down workspace events for the rest of the app.
Suggested fix
onSseEvent: (raw) => {
- handlersRef.current.forEach((handler) => handler(raw.data));
+ handlersRef.current.forEach((handler) => {
+ try {
+ handler(raw.data);
+ } catch (error) {
+ console.error('Workspace event handler failed:', error);
+ }
+ });
},📝 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.
Suggested change
| onSseEvent: (raw) => { | |
| handlersRef.current.forEach((handler) => handler(raw.data)); | |
| }, | |
| onSseEvent: (raw) => { | |
| handlersRef.current.forEach((handler) => { | |
| try { | |
| handler(raw.data); | |
| } catch (error) { | |
| console.error('Workspace event handler failed:', error); | |
| } | |
| }); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/contexts/workspace-events-context.tsx` around lines 38 - 40,
The fan-out in onSseEvent uses handlersRef.current.forEach((handler) =>
handler(raw.data)) so a throwing handler aborts delivery; change this to call
each handler inside its own try/catch (e.g., iterate handlersRef.current and for
each handler invoke handler(raw.data) inside try { ... } catch (err) { /* log
error */ }) so one subscriber failure doesn't stop others, and log the error
with context including the handler identity or event data.
Comment on lines
49
to
56
| return new Promise((resolve) => { | ||
| timeoutRef.current = setTimeout(async () => { | ||
| try { | ||
| const result = await searchFn(query); | ||
| setIsLoading(false); | ||
| setError(null); | ||
| resolve({ data: result, isLoading: false, error: null }); | ||
| } catch (err) { | ||
| const error = err instanceof Error ? err : new Error('Unknown error occurred'); | ||
| setError(error); | ||
| setIsLoading(false); | ||
| resolve({ data: undefined as T, isLoading: false, error }); | ||
| } | ||
| if (inflightQueryRef.current !== query) return; | ||
|
|
||
| const result = await doFetch(query); | ||
| resolve(result); | ||
| }, debounceMs); | ||
| }); |
Contributor
There was a problem hiding this comment.
Promise never settles when query becomes stale.
When a newer query supersedes an older one, line 51 returns early without resolving or rejecting the promise. This leaves callers awaiting search() indefinitely blocked, potentially causing memory leaks and unexpected behaviour.
🐛 Proposed fix: resolve with empty/error result for stale queries
return new Promise((resolve) => {
timeoutRef.current = setTimeout(async () => {
- if (inflightQueryRef.current !== query) return;
+ if (inflightQueryRef.current !== query) {
+ resolve({ data: undefined as T, isLoading: false, error: null });
+ return;
+ }
const result = await doFetch(query);
resolve(result);
}, debounceMs);
});Alternatively, consider rejecting with a specific error type so callers can distinguish cancellation from actual failures.
📝 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.
Suggested change
| return new Promise((resolve) => { | |
| timeoutRef.current = setTimeout(async () => { | |
| try { | |
| const result = await searchFn(query); | |
| setIsLoading(false); | |
| setError(null); | |
| resolve({ data: result, isLoading: false, error: null }); | |
| } catch (err) { | |
| const error = err instanceof Error ? err : new Error('Unknown error occurred'); | |
| setError(error); | |
| setIsLoading(false); | |
| resolve({ data: undefined as T, isLoading: false, error }); | |
| } | |
| if (inflightQueryRef.current !== query) return; | |
| const result = await doFetch(query); | |
| resolve(result); | |
| }, debounceMs); | |
| }); | |
| return new Promise((resolve) => { | |
| timeoutRef.current = setTimeout(async () => { | |
| if (inflightQueryRef.current !== query) { | |
| resolve({ data: undefined as T, isLoading: false, error: null }); | |
| return; | |
| } | |
| const result = await doFetch(query); | |
| resolve(result); | |
| }, debounceMs); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/hooks/use-search-cache.ts` around lines 49 - 56, The returned
Promise in the debounce block (inside use-search-cache's search logic) can
remain unresolved if inflightQueryRef.current !== query; modify that
early-return to settle the Promise by either resolving with a defined "stale"
result (e.g., null or an empty SearchResult) or rejecting with a specific
CancelledError so callers can distinguish cancellation from real failures;
ensure you also clear timeoutRef.current and do any necessary cleanup before
resolving/rejecting, and update callers of search() to handle the chosen
sentinel or error type.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes