feat: make workspace selection global#2015
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces workspace-scoped document and DAG management across the backend and frontend. It adds a workspace query parameter to API endpoints, implements path-prefix filtering in document storage, enables label-based DAG filtering, and updates the UI with workspace selection state management, persistence, and context-aware scoping for documents, DAGs, and design workflows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
ui/src/pages/docs/components/DocEditor.tsx (1)
86-117:⚠️ Potential issue | 🟠 MajorScope saved drafts with the same workspace key as editor state.
useContentEditoris keyed by workspace/remote, butgetDraft,setDraft, andclearDraftstill use onlytabId. Switching workspaces with unsaved edits can restore, overwrite, or clear a draft for the same tab/doc path in another workspace. Use one scoped draft key for all draft operations.🛠️ Suggested direction
+ const scopedDraftKey = `${tabId}:${docPath}:${remoteNode}:${selectedWorkspace || '__all__'}`; + const { currentValue, setCurrentValue, @@ useEffect(() => { - const draft = getDraft(tabId); + const draft = getDraft(scopedDraftKey); if (draft !== undefined) { setCurrentValue(draft); - clearDraft(tabId); + clearDraft(scopedDraftKey); } - }, []); // Only on mount + }, [clearDraft, getDraft, scopedDraftKey, setCurrentValue]); @@ useEffect(() => { return () => { if (hasUnsavedChangesRef.current) { - setDraft(tabId, currentValueRef.current ?? ''); + setDraft(scopedDraftKey, currentValueRef.current ?? ''); } }; - }, [tabId, setDraft]); // Only runs cleanup on tab change or unmount + }, [scopedDraftKey, setDraft]); @@ - clearDraft(tabId); + clearDraft(scopedDraftKey); @@ - clearDraft(tabId); + clearDraft(scopedDraftKey);Also add
scopedDraftKeytohandleSavedependencies when replacingclearDraft(tabId).Also applies to: 150-187, 320-327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/docs/components/DocEditor.tsx` around lines 86 - 117, The draft helpers (getDraft, setDraft, clearDraft) must be scoped by the editor's workspace/remote context to avoid cross-workspace collisions: create a scopedDraftKey (e.g. `${tabId}:${remoteNode}:${selectedWorkspace || '__all__'}`) and replace all calls to getDraft(tabId), setDraft(tabId, ...), and clearDraft(tabId) with getDraft(scopedDraftKey), setDraft(scopedDraftKey, ...), clearDraft(scopedDraftKey); also update the mount/unmount useEffect that currently reads/sets/clears drafts (which uses currentValueRef and hasUnsavedChangesRef) to use the new scopedDraftKey, and add scopedDraftKey to the dependency array for handleSave where clearDraft(tabId) is replaced so the effect and save logic stay consistent with workspace changes.internal/persis/filedag/store.go (1)
786-791:⚠️ Potential issue | 🟠 MajorAdd label scope verification to DAG match retrieval.
SearchCursorenforces label filtering by loading each DAG and checkingcontainsAllLabels(dag.Labels, opts.Labels)before returning results. However,SearchMatchescompletely bypasses this verification—it accepts only afileNameand has noLabelsparameter inSearchDAGMatchesOptions, allowing a caller to retrieve match details for any DAG regardless of label scope restrictions.This creates an authorization bypass: a caller with knowledge of a DAG's filename can request match snippets directly via
SearchMatcheswithout the label filtering applied duringSearchCursor-based discovery.Add a
Labelsfield toSearchDAGMatchesOptionsanddagMatchCursor, then verify the DAG labels inSearchMatchesbefore reading the file—consistent with the label check at line 743 inSearchCursor.Also applies to: 803-818
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/persis/filedag/store.go` around lines 786 - 791, SearchMatches currently bypasses label-scoped access: add a Labels field to the SearchDAGMatchesOptions struct and to dagMatchCursor, populate/encode it where dagMatchCursor is created (e.g., in the SearchCursor path where exec.EncodeSearchCursor is used), and then, inside SearchMatches (the function that reads a DAG given fileName), call containsAllLabels(dag.Labels, opts.Labels) and return an authorization error if the check fails before reading/parsing the file; this mirrors the label check in SearchCursor and ensures SearchMatches honors the same label restrictions.ui/src/pages/dags/dag/index.tsx (2)
1-1:⚠️ Potential issue | 🟡 MinorAdd the required GPL header.
This source file is missing the project license header. As per coding guidelines,
**/*.{go,ts,tsx,js}files must apply GPL v3 license headers managed viamake addlicense.Proposed fix
+// Copyright (C) 2026 Yota Hamada +// SPDX-License-Identifier: GPL-3.0-or-later + import { useCallback, useContext, useEffect, useState } from 'react';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/dags/dag/index.tsx` at line 1, Add the required GPL v3 license header to ui/src/pages/dags/dag/index.tsx by running the project's license tooling: run the make addlicense target (or the repo's documented addlicense command) to insert the standard GPL header at the top of the file, then stage and commit the change; ensure the header appears before any imports (so before the existing import line "import { useCallback, useContext, useEffect, useState } from 'react';") and that the file passes repository lint/license checks.
130-171:⚠️ Potential issue | 🟠 MajorGate DAG-run fetches by the workspace match.
Line 242 hides the DAG UI when it is outside the selected workspace, but Lines 139 and 164 still allow DAG-run/sub-DAG-run API calls once
dagRunNameis known. AdddagMatchesWorkspaceto those enablement checks so non-matching DAGs do not trigger downstream run fetches.Proposed fix
- const dagRunQueryEnabled = Boolean(dagRunName && dagRunId && !subDAGRunId); + const dagRunQueryEnabled = Boolean( + dagMatchesWorkspace && dagRunName && dagRunId && !subDAGRunId + ); ... - const subDAGRunQueryEnabled = Boolean(subDAGRunId && dagRunId && dagRunName); + const subDAGRunQueryEnabled = Boolean( + dagMatchesWorkspace && subDAGRunId && dagRunId && dagRunName + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/dags/dag/index.tsx` around lines 130 - 171, The DAG-run/sub-DAG-run fetches need to be gated by workspace match: update the dagRunQueryEnabled and subDAGRunQueryEnabled booleans to include dagMatchesWorkspace (e.g., dagRunQueryEnabled = Boolean(dagRunName && dagRunId && !subDAGRunId && dagMatchesWorkspace) and subDAGRunQueryEnabled = Boolean(subDAGRunId && dagRunId && dagRunName && dagMatchesWorkspace)) so the useDAGRunSSE/useSubDAGRunSSE and the useQuery calls are only enabled when the DAG matches the selected workspace.ui/src/features/search/components/SearchResult.tsx (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd the required GPL header.
This source file is missing the project license header. As per coding guidelines,
**/*.{go,ts,tsx,js}files must apply GPL v3 license headers managed viamake addlicense.Proposed fix
+// Copyright (C) 2026 Yota Hamada +// SPDX-License-Identifier: GPL-3.0-or-later + import { Button } from '@/components/ui/button';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/search/components/SearchResult.tsx` at line 1, The file SearchResult.tsx is missing the required GPL v3 license header; run the project's license tool or add the header manually: execute "make addlicense" (or apply the same GPL v3 header used across the repo) so the top of ui/src/features/search/components/SearchResult.tsx contains the canonical GPL header comment; ensure the header appears before any imports (e.g., before the existing import { Button } line) and matches other .ts/.tsx files exactly so SearchResult component and module pass license checks.ui/src/hooks/useDocSSE.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd the required GPL header.
This source file is missing the project license header. As per coding guidelines,
**/*.{go,ts,tsx,js}files must apply GPL v3 license headers managed viamake addlicense.Proposed fix
+// Copyright (C) 2026 Yota Hamada +// SPDX-License-Identifier: GPL-3.0-or-later + import { components } from '../api/v1/schema';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/hooks/useDocSSE.ts` at line 1, The file ui/src/hooks/useDocSSE.ts is missing the required GPLv3 license header; add the standard project license header to the top of useDocSSE.ts (before the first import) — either run the repository helper to insert headers (make addlicense) or manually paste the project's GPL v3 header template so the file matches other .ts/.tsx/.js files and CI/license checks; ensure the header appears above the existing import of components and that the change applies to the useDocSSE hook/source file.api/v1/api.yaml (1)
1572-1587:⚠️ Potential issue | 🟠 MajorAdd
Workspaceto the DAG search feed parameters.
/search/docsis workspace-scoped, but/search/dagsstill has noworkspacequery parameter. This can make the global search page return scoped docs alongside unscoped DAG results.🔧 Proposed OpenAPI update
parameters: - $ref: "#/components/parameters/RemoteNode" + - $ref: "#/components/parameters/Workspace" - name: "q" in: "query" required: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.yaml` around lines 1572 - 1587, The /search/dags operation's parameter list is missing the workspace scope; add the existing Workspace query parameter to the parameters array so DAG searches are workspace-scoped like /search/docs. Update the operation that currently references RemoteNode, q, labels, SearchCursor, and SearchLimit to also include the Workspace parameter (reuse the existing components.parameters.Workspace definition) so the OpenAPI spec enforces a workspace query for the DAG search feed.ui/src/features/dags/components/dag-list/DAGTable.tsx (1)
506-514:⚠️ Potential issue | 🟡 MinorStrip workspace labels from active label filters too.
rowLabelsnow excludes workspace labels, butlabelFiltersstill usessearchLabelsverbatim. A persisted or URL-restoredworkspace=...filter will makeevery(...)fail for every row.Proposed fix
- const labelFilters = ( - filterObject?.labels ?? - (Array.isArray(filterValue) ? filterValue : []) - ).map((label) => label.toLowerCase()); + const labelFilters = withoutWorkspaceLabels( + filterObject?.labels ?? + (Array.isArray(filterValue) ? filterValue : []) + ).map((label) => label.toLowerCase());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/dags/components/dag-list/DAGTable.tsx` around lines 506 - 514, labelFilters currently uses the raw active filters and isn't stripping workspace labels like rowLabels does, so apply the same withoutWorkspaceLabels helper to the active filters before lowercasing: pass (filterObject?.labels ?? (Array.isArray(filterValue) ? filterValue : [])) into withoutWorkspaceLabels, then map to .toLowerCase() (similar to how labels -> rowLabels is done) so the comparison between labelFilters and rowLabels works when a workspace=... filter is persisted/restored.ui/src/pages/design/index.tsx (1)
153-174:⚠️ Potential issue | 🟠 MajorGate spec loading by the scoped DAG list.
The DAG list is workspace-filtered, but
selectedDagFilefrom the URL still enables the spec query with onlyremoteNode. A stale or manually crafted/design?dag=...can load/edit a DAG that is not in the selected workspace.Clear the selected DAG when it is absent from the resolved workspace-scoped list, or disable the spec query until membership is confirmed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/design/index.tsx` around lines 153 - 174, The spec query (useQuery('/dags/{fileName}/spec')) is being enabled solely by selectedDagFile and remoteNode, allowing a URL-specified DAG outside the current workspace to load; after dagListQuery resolves, verify that selectedDagFile is present in dagListQuery.data (the workspace-scoped list) and if not clear the selection (call the state setter that controls selectedDagFile) or change the whenEnabled predicate to include a membership check (e.g., whenEnabled(!!selectedDagFile && dagListQuery.data?.includes(selectedDagFile), ...)). Update any related handlers (mutateSpec consumers) to handle the cleared selection.ui/src/contexts/DocTabContext.tsx (1)
65-120:⚠️ Potential issue | 🟠 MajorPrevent doc tabs from leaking across workspace storage keys.
useStateonly readsstorageKeyon mount, while the persistence effect writes the current in-memory tabs to any laterstorageKey. Also, the legacy fallback seeds every non-default key fromdagu_doc_tabs, so a new workspace can inherit unrelated tabs.Reload tab state when
storageKeychanges, or key/remount the provider bystorageKey. Avoid applying the legacy fallback to workspace-specific keys unless this is an explicit one-time migration.ui/src/api/v1/schema.ts (1)
6811-6920:⚠️ Potential issue | 🟠 MajorAdd
workspaceto DAG search query types too.
searchDAGFeedandsearchDagMatchesstill only exposeremoteNode,q, cursor/limit, and labels. Since the PR scopes DAG search by selected workspace, type-safe callers cannot pass the workspace here, so global search can remain unscoped or require unsafe casts.Update the OpenAPI source and regenerate this file.
Suggested generated shape
searchDAGFeed: { parameters: { query: { /** `@description` name of the remote node */ remoteNode?: components["parameters"]["RemoteNode"]; + /** `@description` selected workspace scope */ + workspace?: components["parameters"]["Workspace"]; /** `@description` A search query string */ q: string; /** `@description` Filter DAGs by labels (comma-separated). Returns DAGs that have ALL specified labels. */ labels?: string;searchDagMatches: { parameters: { query: { /** `@description` name of the remote node */ remoteNode?: components["parameters"]["RemoteNode"]; + /** `@description` selected workspace scope */ + workspace?: components["parameters"]["Workspace"]; /** `@description` A search query string */ q: string;As per coding guidelines, Frontend API types must be generated from OpenAPI spec via
pnpm gen:apito maintain type safety with the backend.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/api/v1/schema.ts` around lines 6811 - 6920, The generated API types for searchDAGFeed and searchDagMatches are missing the optional workspace query parameter; update the OpenAPI spec to add workspace (same type as components.parameters.Workspace) to the query object for both searchDAGFeed and searchDagMatches, then run the generator (pnpm gen:api) to regenerate ui/src/api/v1/schema.ts so the searchDAGFeed and searchDagMatches parameter types include workspace and remain type-safe for callers.
🧹 Nitpick comments (5)
internal/persis/fileworkspace/store_test.go (1)
105-111: Consider covering the invalid-characters path explicitly.
"bad/name"exercises the regex mismatch branch ofValidateName, but it would be worth also asserting behavior for the empty-name case (which returns the unwrappedErrInvalidWorkspaceName) and forUpdate/GetByName, since those were also migrated toworkspace.ValidateNameinstore.go. Low priority — existing assertion viaerrors.Isalready tolerates wrapping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/persis/fileworkspace/store_test.go` around lines 105 - 111, Add explicit test cases in TestStore_RejectsInvalidWorkspaceName (or new tests) to cover the empty-name and other validation paths: create a workspace with workspace.NewWorkspace("", "") and assert error is workspace.ErrInvalidWorkspaceName (unwrapped), and also call store.Update(ctx, ...) and store.GetByName(ctx, "") to assert they return the same validation error path; reference workspace.ValidateName, workspace.ErrInvalidWorkspaceName, and the store methods Create/Update/GetByName so the tests exercise both the regex-mismatch and empty-name branches moved to ValidateName.ui/src/hooks/useDocTreeSSE.ts (1)
10-23: Use the generated workspace parameter type for consistency.
workspace?: stringshould beworkspace?: components['parameters']['Workspace']to maintain type safety with the backend schema, matching the pattern used bysort,order, andremoteNode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/hooks/useDocTreeSSE.ts` around lines 10 - 23, The workspace parameter in the useDocTreeSSE function currently uses a plain string; change its type to the generated backend parameter type by replacing workspace?: string with workspace?: components['parameters']['Workspace'] in the params object signature so it matches the other generated types (sort, order, remoteNode) and preserves type-safety for buildSSEEndpoint and useSSE.ui/src/pages/dag-runs/index.tsx (1)
460-465: MemoizeavailableLabelsto matcheffectiveApiLabels.
availableLabelsis recomputed on every render and produces a new array reference each time, which propagates toLabelCombobox(line 841) and theareLabelsEqual/currentFiltersmemo paths indirectly throughselectedLabels. It's inexpensive but the inconsistency with the memoizedeffectiveApiLabelsjust below is easy to fix:♻️ Proposed change
- const availableLabels = withoutWorkspaceLabels(labelsData?.labels ?? []); - - const effectiveApiLabels = React.useMemo( + const availableLabels = React.useMemo( + () => withoutWorkspaceLabels(labelsData?.labels ?? []), + [labelsData?.labels] + ); + + const effectiveApiLabels = React.useMemo( () => withWorkspaceLabel(apiLabels, selectedWorkspace), [apiLabels, selectedWorkspace] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/dag-runs/index.tsx` around lines 460 - 465, availableLabels is recreated on every render which causes unnecessary reference changes; memoize it like effectiveApiLabels by wrapping the call to withoutWorkspaceLabels(labelsData?.labels ?? []) in React.useMemo and add labelsData?.labels (or labelsData) as its dependency so availableLabels only changes when the underlying labels change; this keeps it consistent with effectiveApiLabels (which uses withWorkspaceLabel, apiLabels, selectedWorkspace) and avoids needless re-renders in LabelCombobox and downstream memo paths.ui/src/features/cockpit/components/TemplateSelector.tsx (1)
99-100: Dead alias can be removed.Since client-side workspace filtering was moved server-side,
filteredDagsis now a straight alias ofdags. Either drop the alias and usedagsthroughout, or keep it but note it's vestigial. Not a bug, just cleanup.♻️ Optional simplification
- const dags = data?.dags ?? []; - const filteredDags = dags; + const filteredDags = data?.dags ?? [];(or rename to
dagsand update all downstream usages.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/cockpit/components/TemplateSelector.tsx` around lines 99 - 100, The variable filteredDags in TemplateSelector (where you have const dags = data?.dags ?? []; const filteredDags = dags;) is a redundant alias now that server-side filtering is done; remove the filteredDags declaration and replace all uses of filteredDags in this component with dags (or alternatively keep filteredDags but mark it as vestigial). Update references inside TemplateSelector (JSX and helper functions) to use the single source variable dags and remove the unused filteredDags binding.ui/src/features/cockpit/components/WorkspaceSelector.tsx (1)
1-6: Remove deprecatedMutableRefObjectimport and cast.In React 19,
useRef<T>(initialValue)already returns a mutableRefObject<T>, making theMutableRefObjectcast on line 49 unnecessary. The import on line 5 can also be removed.Suggested cleanup
import React, { useCallback, useRef, useState, - type MutableRefObject, } from 'react';const createStateRef = useRef<'idle' | 'submitted' | 'cancelled'>( 'idle' - ) as MutableRefObject<'idle' | 'submitted' | 'cancelled'>; + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/cockpit/components/WorkspaceSelector.tsx` around lines 1 - 6, Remove the deprecated MutableRefObject type import and the explicit cast where useRef is used in the WorkspaceSelector component: delete MutableRefObject from the React import and remove the cast to MutableRefObject on the ref created with useRef<T>(...), relying on useRef's returned RefObject type instead (update any variable referenced around the existing cast in WorkspaceSelector.tsx to use the inferred ref type).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/exec/context.go`:
- Around line 291-304: The workspaceForDAGDocs function currently trusts
user-controlled workspace label values which can escape the intended
DAG_DOCS_DIR when later joined; modify workspaceForDAGDocs (and the similar
logic used around EnvKeyDAGDocsDir) to validate each label value before
accepting it: either call the existing backend workspace validator function or
enforce that the value is a single safe path segment (reject empty values, any
path separators like '/' or '\\', and any '.' or '..' segments, and disallow
leading/trailing whitespace), and only return a workspaceName when it passes
validation and is consistent across labels. Ensure you reference
workspaceForDAGDocs and the code paths that use EnvKeyDAGDocsDir so the
validated name is used for filepath.Join.
In `@internal/service/frontend/api/v1/workspaces.go`:
- Around line 128-136: The update handler currently skips validation when
body.Name is provided as an explicit empty string; change the condition that
guards validation from "if body.Name != nil && *body.Name != \"\"" to simply "if
body.Name != nil" so that workspace.ValidateName(*body.Name) runs for provided
empty strings as well; if ValidateName returns an error, return the existing
&Error{Code: api.ErrorCodeBadRequest, Message: "...", HTTPStatus:
http.StatusBadRequest} and do not assign existing.Name = *body.Name. Ensure you
reference body.Name, workspace.ValidateName, and existing.Name when making the
change.
In `@ui/src/App.tsx`:
- Around line 244-324: Replace the raw fetchWithTimeout + response.json usage in
fetchWorkspaces, handleCreateWorkspace, and handleDeleteWorkspace with the
OpenAPI-generated client calls (use client.GET / client.POST / client.DELETE or
the generated client.workspaces.list/create/delete methods) and pass remoteNode
as a query param to those calls; use the typed response from the client directly
(no response.json parsing) and keep the existing error handling
(setWorkspaceError, throwing on create failure) and state updates
(applyWorkspaces, handleSelectWorkspace, workspaces fallback for local). Ensure
you remove manual URL building with config.apiURL for these three functions and
import/use the generated client methods so frontend API types come from the
OpenAPI-generated client.
- Around line 244-270: The fetchWorkspaces function can apply stale responses
after selectedRemoteNode changes; capture the node (e.g., const expectedNode =
selectedRemoteNode) at the top of fetchWorkspaces and, before calling
applyWorkspaces, setWorkspaceError, and setWorkspacesLoaded, check that
selectedRemoteNode === expectedNode (or otherwise abort) so responses for
earlier nodes are ignored; apply this guard for success, error, and finally
branches (alternatively use an AbortController tied to the current
selectedRemoteNode and abort previous requests).
In `@ui/src/contexts/DocTabContext.tsx`:
- Line 1: This file (DocTabContext.tsx) is missing the required GPL v3 license
header; add the standard GPL v3 header comment block to the very top of the file
(above the import React line) matching the repository's license header format
used by make addlicense so it complies with the rule for **/*.{go,ts,tsx,js};
ensure the header is identical to other TSX files in the repo.
In `@ui/src/features/cockpit/hooks/useCockpitState.ts`:
- Line 1: This file (the React hook useCockpitState in useCockpitState.ts) is
missing the project's GPLv3 license header; run the repository license tool
(make addlicense) or manually add the standard GPL v3 header comment at the top
of the file (above the import line "import { useContext, useEffect, useState }
from 'react';") so the file includes the same GPL v3 header used across other
.ts/.tsx/.js files.
In `@ui/src/features/search/components/SearchResult.tsx`:
- Around line 178-188: The pagination call in loadMore (the async loadMore:
(cursor?: string) => Promise<LoadMoreResponse>) must be scoped by workspace
labels: update the backend OpenAPI to accept a labels (or workspaceLabel) query
parameter on '/search/dags/{fileName}/matches', then import workspaceLabel from
'@/lib/workspace' and include it in the client.GET params.query (alongside
remoteNode, q, cursor) so the Show more matches request uses the same workspace
filtering as the initial /search/dags call; keep using result.fileName for the
path param.
In `@ui/src/hooks/useDocSSE.ts`:
- Around line 8-17: The SSE docs endpoint in useDocSSE is missing the remoteNode
query param, so modify the hook signature to accept remoteNode (e.g., add
remoteNode?: string to useDocSSE) and ensure params.set('remoteNode', remoteNode
|| 'local') (or equivalent) is always added before building endpoint; update the
endpoint construction that creates `endpoint` (the
`/events/docs/${encodedPath}${query ? `?${query}` : ''}` line) to include this
param and then update all call sites to pass appBarContext.selectedRemoteNode ||
'local' when invoking useDocSSE.
In `@ui/src/lib/dagSpec.ts`:
- Around line 28-49: The fallback in ensureWorkspaceLabelInDAGSpec can create
duplicate top-level labels and replace mapping labels with a sequence; change
the behavior so that on parseDocument errors you do not blindly prepend via
prependWorkspaceLabel but instead return the original spec (so callers/UI can
warn) or, if you must modify, first detect an existing top-level "labels:"
(e.g., a safe regex/line-scan for "^labels\\s*:") and avoid prepending;
additionally update the merge logic that uses labelsFromNode and doc.set so it
preserves mapping nodes (detect map vs seq in the parsed node, convert/merge the
workspace label into an existing mapping rather than replacing it with an array,
or append to an existing sequence) by handling mapping nodes from
labelsFromNode/withWorkspaceLabel and using doc.set only after merging
correctly.
In `@ui/src/lib/workspace.ts`:
- Around line 11-71: sanitizeWorkspaceName should no longer silently strip
invalid characters — change it to only trim (or otherwise normalize without
removing characters) and rely on isValidWorkspaceName to enforce
WORKSPACE_NAME_PATTERN; update workspaceLabel(name) to return undefined if the
raw name differs from the sanitized value or fails isValidWorkspaceName so we
reject names like "prod!" instead of mapping them to "prod". Also update
getStoredWorkspaceName to read stored and legacy values, run them through the
new sanitize + isValidWorkspaceName check before returning or migrating (only
persist legacy if valid), and change persistWorkspaceName to only set
localStorage when the provided name is valid (remove the key when invalid/empty)
and always clear LEGACY_COCKPIT_WORKSPACE_STORAGE_KEY; use the functions
sanitizeWorkspaceName, isValidWorkspaceName, workspaceLabel,
getStoredWorkspaceName, and persistWorkspaceName to locate the changes.
In `@ui/src/pages/dags/index.tsx`:
- Around line 68-70: The current sentinel string "__all__" causes collisions
with a real workspace named "__all__"; instead encode the workspace scope
structurally. Replace the logic around appBarContext.selectedWorkspace ->
workspaceScopeKey and searchStateScope so workspaceScopeKey is a structured,
unambiguous string (for example JSON.stringify of an object like
{type:'workspace', id: selectedWorkspace ?? null} or similar) and then compose
searchStateScope using remoteNode plus that encoded structural key; update any
consumers expecting the old sentinel to read the new structured key. Ensure you
reference and change the variables named selectedWorkspace, workspaceScopeKey
and searchStateScope (and use remoteNode/appBarContext.selectedWorkspace) so
persisted keys are collision-safe.
In `@ui/src/pages/docs/components/DocEditor.tsx`:
- Line 57: The SSE subscription is not routed by remoteNode, causing stale
updates for non-local docs; update the useDocSSE hook to accept a remoteNode
parameter and include it in the SSE endpoint/query construction, then change the
call site in DocEditor to determine the selected remote node (e.g., from
appBarContext.selectedRemoteNode defaulting to 'local') and call
useDocSSE(docPath, !!docPath, workspaceQuery, remoteNode) so the SSE endpoint
matches the GET/PATCH routing used elsewhere (refer to useDocSSE, docPath,
workspaceQuery, and appBarContext.selectedRemoteNode).
In `@ui/src/pages/search/index.tsx`:
- Around line 376-378: The current scope key can collide with a real workspace
named "__all__"; change the key construction to a structured, collision-safe
representation using the existing symbols selectedWorkspace, workspaceScopeKey
and remoteKey—for example replace workspaceScopeKey = selectedWorkspace ||
'__all__' and searchStateScope = `${remoteKey}:${workspaceScopeKey}` with a
structured form like searchStateScope = JSON.stringify({ remote: remoteKey,
workspace: selectedWorkspace || '__all__' }) (or an equivalent deterministic
encoder) so the remote/workspace pair is unambiguous and cannot collide with a
workspace name.
---
Outside diff comments:
In `@api/v1/api.yaml`:
- Around line 1572-1587: The /search/dags operation's parameter list is missing
the workspace scope; add the existing Workspace query parameter to the
parameters array so DAG searches are workspace-scoped like /search/docs. Update
the operation that currently references RemoteNode, q, labels, SearchCursor, and
SearchLimit to also include the Workspace parameter (reuse the existing
components.parameters.Workspace definition) so the OpenAPI spec enforces a
workspace query for the DAG search feed.
In `@internal/persis/filedag/store.go`:
- Around line 786-791: SearchMatches currently bypasses label-scoped access: add
a Labels field to the SearchDAGMatchesOptions struct and to dagMatchCursor,
populate/encode it where dagMatchCursor is created (e.g., in the SearchCursor
path where exec.EncodeSearchCursor is used), and then, inside SearchMatches (the
function that reads a DAG given fileName), call containsAllLabels(dag.Labels,
opts.Labels) and return an authorization error if the check fails before
reading/parsing the file; this mirrors the label check in SearchCursor and
ensures SearchMatches honors the same label restrictions.
In `@ui/src/api/v1/schema.ts`:
- Around line 6811-6920: The generated API types for searchDAGFeed and
searchDagMatches are missing the optional workspace query parameter; update the
OpenAPI spec to add workspace (same type as components.parameters.Workspace) to
the query object for both searchDAGFeed and searchDagMatches, then run the
generator (pnpm gen:api) to regenerate ui/src/api/v1/schema.ts so the
searchDAGFeed and searchDagMatches parameter types include workspace and remain
type-safe for callers.
In `@ui/src/features/dags/components/dag-list/DAGTable.tsx`:
- Around line 506-514: labelFilters currently uses the raw active filters and
isn't stripping workspace labels like rowLabels does, so apply the same
withoutWorkspaceLabels helper to the active filters before lowercasing: pass
(filterObject?.labels ?? (Array.isArray(filterValue) ? filterValue : [])) into
withoutWorkspaceLabels, then map to .toLowerCase() (similar to how labels ->
rowLabels is done) so the comparison between labelFilters and rowLabels works
when a workspace=... filter is persisted/restored.
In `@ui/src/features/search/components/SearchResult.tsx`:
- Line 1: The file SearchResult.tsx is missing the required GPL v3 license
header; run the project's license tool or add the header manually: execute "make
addlicense" (or apply the same GPL v3 header used across the repo) so the top of
ui/src/features/search/components/SearchResult.tsx contains the canonical GPL
header comment; ensure the header appears before any imports (e.g., before the
existing import { Button } line) and matches other .ts/.tsx files exactly so
SearchResult component and module pass license checks.
In `@ui/src/hooks/useDocSSE.ts`:
- Line 1: The file ui/src/hooks/useDocSSE.ts is missing the required GPLv3
license header; add the standard project license header to the top of
useDocSSE.ts (before the first import) — either run the repository helper to
insert headers (make addlicense) or manually paste the project's GPL v3 header
template so the file matches other .ts/.tsx/.js files and CI/license checks;
ensure the header appears above the existing import of components and that the
change applies to the useDocSSE hook/source file.
In `@ui/src/pages/dags/dag/index.tsx`:
- Line 1: Add the required GPL v3 license header to
ui/src/pages/dags/dag/index.tsx by running the project's license tooling: run
the make addlicense target (or the repo's documented addlicense command) to
insert the standard GPL header at the top of the file, then stage and commit the
change; ensure the header appears before any imports (so before the existing
import line "import { useCallback, useContext, useEffect, useState } from
'react';") and that the file passes repository lint/license checks.
- Around line 130-171: The DAG-run/sub-DAG-run fetches need to be gated by
workspace match: update the dagRunQueryEnabled and subDAGRunQueryEnabled
booleans to include dagMatchesWorkspace (e.g., dagRunQueryEnabled =
Boolean(dagRunName && dagRunId && !subDAGRunId && dagMatchesWorkspace) and
subDAGRunQueryEnabled = Boolean(subDAGRunId && dagRunId && dagRunName &&
dagMatchesWorkspace)) so the useDAGRunSSE/useSubDAGRunSSE and the useQuery calls
are only enabled when the DAG matches the selected workspace.
In `@ui/src/pages/design/index.tsx`:
- Around line 153-174: The spec query (useQuery('/dags/{fileName}/spec')) is
being enabled solely by selectedDagFile and remoteNode, allowing a URL-specified
DAG outside the current workspace to load; after dagListQuery resolves, verify
that selectedDagFile is present in dagListQuery.data (the workspace-scoped list)
and if not clear the selection (call the state setter that controls
selectedDagFile) or change the whenEnabled predicate to include a membership
check (e.g., whenEnabled(!!selectedDagFile &&
dagListQuery.data?.includes(selectedDagFile), ...)). Update any related handlers
(mutateSpec consumers) to handle the cleared selection.
In `@ui/src/pages/docs/components/DocEditor.tsx`:
- Around line 86-117: The draft helpers (getDraft, setDraft, clearDraft) must be
scoped by the editor's workspace/remote context to avoid cross-workspace
collisions: create a scopedDraftKey (e.g.
`${tabId}:${remoteNode}:${selectedWorkspace || '__all__'}`) and replace all
calls to getDraft(tabId), setDraft(tabId, ...), and clearDraft(tabId) with
getDraft(scopedDraftKey), setDraft(scopedDraftKey, ...),
clearDraft(scopedDraftKey); also update the mount/unmount useEffect that
currently reads/sets/clears drafts (which uses currentValueRef and
hasUnsavedChangesRef) to use the new scopedDraftKey, and add scopedDraftKey to
the dependency array for handleSave where clearDraft(tabId) is replaced so the
effect and save logic stay consistent with workspace changes.
---
Nitpick comments:
In `@internal/persis/fileworkspace/store_test.go`:
- Around line 105-111: Add explicit test cases in
TestStore_RejectsInvalidWorkspaceName (or new tests) to cover the empty-name and
other validation paths: create a workspace with workspace.NewWorkspace("", "")
and assert error is workspace.ErrInvalidWorkspaceName (unwrapped), and also call
store.Update(ctx, ...) and store.GetByName(ctx, "") to assert they return the
same validation error path; reference workspace.ValidateName,
workspace.ErrInvalidWorkspaceName, and the store methods Create/Update/GetByName
so the tests exercise both the regex-mismatch and empty-name branches moved to
ValidateName.
In `@ui/src/features/cockpit/components/TemplateSelector.tsx`:
- Around line 99-100: The variable filteredDags in TemplateSelector (where you
have const dags = data?.dags ?? []; const filteredDags = dags;) is a redundant
alias now that server-side filtering is done; remove the filteredDags
declaration and replace all uses of filteredDags in this component with dags (or
alternatively keep filteredDags but mark it as vestigial). Update references
inside TemplateSelector (JSX and helper functions) to use the single source
variable dags and remove the unused filteredDags binding.
In `@ui/src/features/cockpit/components/WorkspaceSelector.tsx`:
- Around line 1-6: Remove the deprecated MutableRefObject type import and the
explicit cast where useRef is used in the WorkspaceSelector component: delete
MutableRefObject from the React import and remove the cast to MutableRefObject
on the ref created with useRef<T>(...), relying on useRef's returned RefObject
type instead (update any variable referenced around the existing cast in
WorkspaceSelector.tsx to use the inferred ref type).
In `@ui/src/hooks/useDocTreeSSE.ts`:
- Around line 10-23: The workspace parameter in the useDocTreeSSE function
currently uses a plain string; change its type to the generated backend
parameter type by replacing workspace?: string with workspace?:
components['parameters']['Workspace'] in the params object signature so it
matches the other generated types (sort, order, remoteNode) and preserves
type-safety for buildSSEEndpoint and useSSE.
In `@ui/src/pages/dag-runs/index.tsx`:
- Around line 460-465: availableLabels is recreated on every render which causes
unnecessary reference changes; memoize it like effectiveApiLabels by wrapping
the call to withoutWorkspaceLabels(labelsData?.labels ?? []) in React.useMemo
and add labelsData?.labels (or labelsData) as its dependency so availableLabels
only changes when the underlying labels change; this keeps it consistent with
effectiveApiLabels (which uses withWorkspaceLabel, apiLabels, selectedWorkspace)
and avoids needless re-renders in LabelCombobox and downstream memo paths.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25f997bf-d41d-4e40-8240-d790253864c3
📒 Files selected for processing (43)
api/v1/api.gen.goapi/v1/api.yamlinternal/agent/doc.gointernal/core/exec/context.gointernal/core/exec/context_test.gointernal/core/exec/dag.gointernal/persis/filedag/store.gointernal/persis/filedag/store_test.gointernal/persis/filedoc/store.gointernal/persis/filedoc/store_test.gointernal/persis/fileworkspace/store.gointernal/persis/fileworkspace/store_test.gointernal/service/frontend/api/v1/docs.gointernal/service/frontend/api/v1/search.gointernal/service/frontend/api/v1/workspaces.gointernal/service/frontend/sse/topic_parse.gointernal/workspace/workspace.goui/src/App.tsxui/src/api/v1/schema.tsui/src/contexts/AppBarContext.tsui/src/contexts/DocTabContext.tsxui/src/features/cockpit/components/CockpitToolbar.tsxui/src/features/cockpit/components/TemplateSelector.tsxui/src/features/cockpit/components/WorkspaceSelector.tsxui/src/features/cockpit/hooks/useCockpitState.tsui/src/features/dags/components/common/CreateDAGModal.tsxui/src/features/dags/components/dag-list/DAGTable.tsxui/src/features/search/components/SearchResult.tsxui/src/hooks/SSEManager.tsui/src/hooks/useDocSSE.tsui/src/hooks/useDocTreeSSE.tsui/src/lib/dagSpec.tsui/src/lib/workspace.tsui/src/menu.tsxui/src/pages/cockpit/index.tsxui/src/pages/dag-runs/index.tsxui/src/pages/dags/dag/index.tsxui/src/pages/dags/index.tsxui/src/pages/design/buildWorkflowDesignPrompt.tsui/src/pages/design/index.tsxui/src/pages/docs/components/DocEditor.tsxui/src/pages/docs/index.tsxui/src/pages/search/index.tsx
💤 Files with no reviewable changes (1)
- ui/src/features/cockpit/components/CockpitToolbar.tsx
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
1 participant
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
Testing
Summary by CodeRabbit