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:
📝 WalkthroughWalkthroughIntroduces workspace-scoped base configuration API endpoints and loading infrastructure. Adds GET/PUT endpoints for managing workspace-specific DAG base configurations that merge with global base configs. Extends DAG loading, git sync, and UI to support this new scoped configuration level. ChangesWorkspace-Scoped Base Configuration Feature
Sequence DiagramsequenceDiagram
participant UI as Frontend UI
participant API as API Handler
participant Loader as DAG Loader
participant Store as Workspace<br/>Config Store
participant FS as Filesystem
rect rgba(100, 149, 237, 0.5)
note over UI,FS: Load DAG with Workspace Base Config Merge
UI->>API: Load DAG (workspace context)
API->>Loader: LoadDAG with<br/>WorkspaceBaseConfigDir
Loader->>FS: Read global base.yaml
FS-->>Loader: Global base definition
Loader->>Store: Get workspace base<br/>config directory
Store->>FS: Read workspace/base.yaml
FS-->>Store: Workspace base definition
Store-->>Loader: Workspace base definition
Loader->>Loader: Merge global base<br/>+ workspace base
Loader->>FS: Read DAG spec
FS-->>Loader: DAG spec
Loader->>Loader: Apply merged base<br/>to DAG
Loader-->>API: Resolved DAG
API-->>UI: DAG with merged config
end
rect rgba(144, 238, 144, 0.5)
note over UI,FS: Update Workspace Base Config
UI->>API: PUT new base config
API->>API: Validate spec
API->>Store: Save to workspace<br/>config store
Store->>FS: Write workspace/base.yaml
FS-->>Store: Write complete
Store-->>API: Saved
API-->>UI: Success response
UI->>UI: Refresh DAG view
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/src/pages/git-sync/MoveDialog.tsx (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the required GPL license header to this TSX file.
This changed source file is missing the repository-required GPL v3 header block.
As per coding guidelines `**/*.{go,ts,tsx,js}: Apply GPL v3 license headers on source files, managed via make addlicense`.🔧 Suggested fix
+// Copyright (C) 2026 Yota Hamada +// SPDX-License-Identifier: GPL-3.0-or-later + import { SyncStatus } from '@/api/v1/schema';🤖 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 `@ui/src/pages/git-sync/MoveDialog.tsx` at line 1, This file is missing the repository-required GPL v3 header; add the standard GPL v3 license header block to the top of the MoveDialog TSX module (above the import of SyncStatus) so the MoveDialog React component file includes the required header; best approach is to run the repo tool to apply headers (e.g., run "make addlicense" or follow the project's addlicense convention) so the correct GPL v3 block is inserted consistently.
🧹 Nitpick comments (2)
internal/gitsync/service_test.go (1)
79-86: ⚡ Quick winConsider adding a comment explaining why
"workspaces/default/base"maps toDAGKindDAG.The assertion at Line 84 correctly captures that
"default"is not a valid workspace name perworkspace.ValidateName, so it falls through toDAGKindDAG. Without a brief comment, a future reader may wonder why an ID that structurally looks like a workspace config path (3-segment, ending in"base") does not receiveDAGKindConfig.💬 Suggested comment
- assert.Equal(t, DAGKindDAG, KindForDAGID("workspaces/default/base")) + // "default" is rejected by workspace.ValidateName, so this is treated as a regular DAG ID. + assert.Equal(t, DAGKindDAG, KindForDAGID("workspaces/default/base"))🤖 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 `@internal/gitsync/service_test.go` around lines 79 - 86, Add a brief inline comment in the TestKindForDAGID_ConfigFiles test (near the assertion for "workspaces/default/base") explaining that KindForDAGID maps that ID to DAGKindDAG because "default" is rejected by workspace.ValidateName (i.e., it's not a valid workspace name), so the code falls through to treat it as a DAG rather than a config; reference the test name TestKindForDAGID_ConfigFiles and the KindForDAGID function in the comment for clarity.internal/gitsync/service.go (1)
511-527: 💤 Low valueHardcoded
.yamltrim couples producer and parser to a specific filename convention.Both the scanner (
scanConfigFilesat line 524) and the parser (workspaceBaseConfigNameFromIDin state.go:106) usestrings.TrimSuffix(workspace.BaseConfigFileName, ".yaml"). IfBaseConfigFileNameever changes extension (e.g., from"base.yaml"to"base.yml"), both locations must be updated simultaneously. Failure to do so creates a silent mismatch where scanner and parser diverge, orphaning or unmapping workspace configs.Consider deriving the suffix from
path.Ext(workspace.BaseConfigFileName)or exposing a shared helper in theworkspacepackage, so both producer and consumer reference one source of truth.🤖 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 `@internal/gitsync/service.go` around lines 511 - 527, The scanner in scanConfigFiles uses strings.TrimSuffix(workspace.BaseConfigFileName, ".yaml") which hardcodes the extension and can diverge from workspaceBaseConfigNameFromID; change both sides to use a single source of truth by either deriving the suffix via path.Ext(workspace.BaseConfigFileName) or adding a helper in the workspace package (e.g., WorkspaceBaseConfigStem or TrimBaseConfigExt) and update scanConfigFiles and workspaceBaseConfigNameFromID to call that helper so producer and parser always agree on the base config name.
🤖 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 `@api/v1/api.yaml`:
- Around line 5149-5153: Replace the plain string schema for the path parameter
named workspaceName with a $ref to the existing components schema WorkspaceName;
locate the parameter definition for workspaceName (currently "schema: type:
string") and change it to "schema: $ref: '#/components/schemas/WorkspaceName'"
for both occurrences (the one at the top of the diff and the other occurrence
around the second mention), ensuring the parameter name remains workspaceName
and the ref exactly matches the existing WorkspaceName schema symbol.
In `@internal/core/spec/loader.go`:
- Around line 414-423: loadDAGsFromData currently resolves workspace/base
per-document so later documents in a multi-doc file can miss the workspace and
get a different BaseConfigData; change loadDAGsFromData to resolve the
workspace/base once for the file (using the primary/first document after
decodeDocuments) and then pass that resolved baseDef/baseRaw into
processDAGDocument for subsequent docs when they don't declare workspace labels;
update the loop in loadDAGsFromData (and the analogous logic around
processDAGDocument at the other mentioned site) to check each doc for an
explicit workspace and otherwise reuse the file-level baseDef/baseRaw so all
documents in the file inherit the same workspace base.
- Around line 640-687: mergeDefinitionMaps currently only deep-merges nested
map[string]any and overwrites when a field is a list (e.g., env written as YAML
list), causing workspace base lists to replace rather than layer on global
lists; fix by normalizing union fields before merging: in
mergeBaseDefinitionData, decode/normalize baseRaw and overrideRaw into their
canonical dag/structured form (using decode or decodeDefinitionData) or add
logic in mergeDefinitionMaps to special-case known union keys (e.g., "env") —
detect when baseValue or overrideValue is a slice vs map and convert list syntax
into the canonical map/list representation (or merge by name/append deduplicated
entries) before assigning merged[key]; ensure mergedRaw is produced from the
normalized merged structure so YAML syntax differences no longer affect
semantics (refer to mergeBaseDefinitionData, mergeDefinitionMaps, decode,
decodeDefinitionData).
In `@internal/service/frontend/api/v1/dagruns.go`:
- Around line 4258-4261: The default reschedule path rebuilds the DAG using
loadInlineDAG(snapshotDAG.YamlData) without reapplying workspace or preserved
base-config layers, so when rescheduling a named-workspace DAG workspace-scoped
settings are lost; update the rebuild to pass the same spec.LoadOption values as
the useCurrentDAGFile branch (include
spec.WithWorkspaceBaseConfigDir(workspace.BaseConfigDir(...)) and
spec.WithBaseConfig populated from snapshotDAG.BaseConfigData or the persisted
base config) or change loadInlineDAG to accept and apply those load options and
honor snapshotDAG.BaseConfigData so the workspace layer and preserved
base-config are reapplied during the snapshot-based load.
In `@internal/workspace/workspace.go`:
- Around line 33-38: The BaseConfigPath function currently builds a path with
the raw name which allows names like "../other" to escape the workspaces
directory; call ValidateName(name) (or equivalent workspace name validator) at
the start of BaseConfigPath and return "" if validation fails so only validated
names are joined (i.e., add a guard: if !ValidateName(name) return "") before
calling BaseConfigDir and filepath.Join.
In `@ui/src/pages/base-config/index.tsx`:
- Around line 87-90: The global scope isn't reset when remoteNode (or
selectedWorkspace) changes, leaving stale globalValue/globalDirty and risking
cross-node saves; add a useEffect that listens to [selectedWorkspace,
remoteNode] and calls setGlobalValue(null) and setGlobalDirty(false) (mirroring
the workspace reset) so globalValue will be re-seeded from globalData for the
new node; reference the existing state setters setGlobalValue and setGlobalDirty
and the globalData/init effect that seeds globalValue when null.
---
Outside diff comments:
In `@ui/src/pages/git-sync/MoveDialog.tsx`:
- Line 1: This file is missing the repository-required GPL v3 header; add the
standard GPL v3 license header block to the top of the MoveDialog TSX module
(above the import of SyncStatus) so the MoveDialog React component file includes
the required header; best approach is to run the repo tool to apply headers
(e.g., run "make addlicense" or follow the project's addlicense convention) so
the correct GPL v3 block is inserted consistently.
---
Nitpick comments:
In `@internal/gitsync/service_test.go`:
- Around line 79-86: Add a brief inline comment in the
TestKindForDAGID_ConfigFiles test (near the assertion for
"workspaces/default/base") explaining that KindForDAGID maps that ID to
DAGKindDAG because "default" is rejected by workspace.ValidateName (i.e., it's
not a valid workspace name), so the code falls through to treat it as a DAG
rather than a config; reference the test name TestKindForDAGID_ConfigFiles and
the KindForDAGID function in the comment for clarity.
In `@internal/gitsync/service.go`:
- Around line 511-527: The scanner in scanConfigFiles uses
strings.TrimSuffix(workspace.BaseConfigFileName, ".yaml") which hardcodes the
extension and can diverge from workspaceBaseConfigNameFromID; change both sides
to use a single source of truth by either deriving the suffix via
path.Ext(workspace.BaseConfigFileName) or adding a helper in the workspace
package (e.g., WorkspaceBaseConfigStem or TrimBaseConfigExt) and update
scanConfigFiles and workspaceBaseConfigNameFromID to call that helper so
producer and parser always agree on the base config name.
🪄 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: 9a0a3401-03ba-4cec-8040-f9062b45f88d
📒 Files selected for processing (32)
api/v1/api.gen.goapi/v1/api.yamlinternal/agentsnapshot/dispatch.gointernal/cmd/context.gointernal/cmd/dry.gointernal/cmd/start.gointernal/cmd/stop.gointernal/cmd/sync.gointernal/cmd/validate.gointernal/core/spec/builder.gointernal/core/spec/loader.gointernal/core/spec/loader_test.gointernal/engine/engine.gointernal/engine/run.gointernal/gitsync/service.gointernal/gitsync/service_test.gointernal/gitsync/state.gointernal/persis/filedag/store.gointernal/persis/filedag/store_test.gointernal/service/frontend/api/v1/base_config.gointernal/service/frontend/api/v1/dagruns.gointernal/service/frontend/api/v1/sync.gointernal/service/frontend/api/v1/sync_test.gointernal/service/frontend/server.gointernal/test/helper.gointernal/test/scheduler.gointernal/workspace/workspace.goui/src/api/v1/schema.tsui/src/pages/base-config/index.tsxui/src/pages/git-sync/MoveDialog.tsxui/src/pages/git-sync/RowActionMenu.tsxui/src/pages/git-sync/index.tsx
Comment on lines
+5149
to
+5153
| - name: workspaceName | ||
| in: path | ||
| required: true | ||
| schema: | ||
| type: string |
There was a problem hiding this comment.
Constrain workspaceName with the existing WorkspaceName schema.
On Line 5149 and Line 5212, workspaceName is a plain string, but this endpoint is for named workspaces only. Reusing #/components/schemas/WorkspaceName keeps reserved-name validation (all, default) consistent with the rest of the API contract.
Suggested diff
parameters:
- name: workspaceName
in: path
required: true
schema:
- type: string
+ $ref: "#/components/schemas/WorkspaceName"
- $ref: "#/components/parameters/RemoteNode"
...
parameters:
- name: workspaceName
in: path
required: true
schema:
- type: string
+ $ref: "#/components/schemas/WorkspaceName"
- $ref: "#/components/parameters/RemoteNode"Also applies to: 5212-5216
🤖 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 `@api/v1/api.yaml` around lines 5149 - 5153, Replace the plain string schema
for the path parameter named workspaceName with a $ref to the existing
components schema WorkspaceName; locate the parameter definition for
workspaceName (currently "schema: type: string") and change it to "schema: $ref:
'#/components/schemas/WorkspaceName'" for both occurrences (the one at the top
of the diff and the other occurrence around the second mention), ensuring the
parameter name remains workspaceName and the ref exactly matches the existing
WorkspaceName schema symbol.
Comment on lines
+414
to
423
| func loadDAGsFromData(ctx BuildContext, data []byte, filePath string, baseDef *dag, baseRaw []byte) ([]*core.DAG, error) { | ||
| docs, err := decodeDocuments(data) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| dags := make([]*core.DAG, 0, len(docs)) | ||
| for _, doc := range docs { | ||
| dag, err := processDAGDocument(buildDocumentContext(ctx, doc.index), doc.data, baseDef, filePath, data) | ||
| dag, err := processDAGDocument(buildDocumentContext(ctx, doc.index), doc.data, baseDef, baseRaw, filePath, data) | ||
| if err != nil { |
There was a problem hiding this comment.
Propagate the workspace base config to every document in a multi-document DAG file.
The workspace layer is resolved from each document independently, so only documents that repeat labels/tags: workspace=... inherit it. Inline local DAG documents usually omit those labels, which means a parent DAG can build with the workspace base while its local sub-DAGs fall back to the global base and carry different BaseConfigData. Please resolve the workspace once per file, or at least fall back to the primary document's workspace when later documents omit it.
Also applies to: 515-516
🤖 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 `@internal/core/spec/loader.go` around lines 414 - 423, loadDAGsFromData
currently resolves workspace/base per-document so later documents in a multi-doc
file can miss the workspace and get a different BaseConfigData; change
loadDAGsFromData to resolve the workspace/base once for the file (using the
primary/first document after decodeDocuments) and then pass that resolved
baseDef/baseRaw into processDAGDocument for subsequent docs when they don't
declare workspace labels; update the loop in loadDAGsFromData (and the analogous
logic around processDAGDocument at the other mentioned site) to check each doc
for an explicit workspace and otherwise reuse the file-level baseDef/baseRaw so
all documents in the file inherit the same workspace base.
Comment on lines
+640
to
+687
| func mergeBaseDefinitionData(baseRaw, overrideRaw []byte) (*dag, []byte, error) { | ||
| if len(baseRaw) == 0 { | ||
| def, err := decodeDefinitionData(overrideRaw, "workspace base config") | ||
| return def, overrideRaw, err | ||
| } | ||
| if len(overrideRaw) == 0 { | ||
| def, err := decodeDefinitionData(baseRaw, "base config") | ||
| return def, baseRaw, err | ||
| } | ||
|
|
||
| baseMap, err := unmarshalData(baseRaw) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("failed to unmarshal base config: %w", err) | ||
| } | ||
| overrideMap, err := unmarshalData(overrideRaw) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("failed to unmarshal workspace base config: %w", err) | ||
| } | ||
|
|
||
| mergedMap := mergeDefinitionMaps(baseMap, overrideMap) | ||
| def, err := decode(mergedMap) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("failed to decode merged base config: %w", err) | ||
| } | ||
|
|
||
| mergedRaw, err := yaml.Marshal(mergedMap) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("failed to marshal merged base config: %w", err) | ||
| } | ||
| return def, mergedRaw, nil | ||
| } | ||
|
|
||
| func mergeDefinitionMaps(base, override map[string]any) map[string]any { | ||
| merged := cloneMap(base) | ||
| if merged == nil { | ||
| merged = make(map[string]any, len(override)) | ||
| } | ||
| for key, overrideValue := range override { | ||
| baseValue, ok := merged[key] | ||
| baseMap, baseIsMap := baseValue.(map[string]any) | ||
| overrideMap, overrideIsMap := overrideValue.(map[string]any) | ||
| if ok && baseIsMap && overrideIsMap { | ||
| merged[key] = mergeDefinitionMaps(baseMap, overrideMap) | ||
| continue | ||
| } | ||
| merged[key] = cloneAny(overrideValue) | ||
| } | ||
| return merged |
There was a problem hiding this comment.
Raw map merging drops inherited values when base configs use list syntax.
mergeDefinitionMaps only deep-merges nested map[string]any. Supported manifest unions like env can also be written as lists, and those hit the overwrite path at Line 685, so a workspace base can silently replace global entries instead of layering on top of them. That makes the merged result depend on YAML syntax rather than config meaning. Please merge after decoding/normalizing these union fields, or special-case them before re-encoding the merged raw YAML.
🤖 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 `@internal/core/spec/loader.go` around lines 640 - 687, mergeDefinitionMaps
currently only deep-merges nested map[string]any and overwrites when a field is
a list (e.g., env written as YAML list), causing workspace base lists to replace
rather than layer on global lists; fix by normalizing union fields before
merging: in mergeBaseDefinitionData, decode/normalize baseRaw and overrideRaw
into their canonical dag/structured form (using decode or decodeDefinitionData)
or add logic in mergeDefinitionMaps to special-case known union keys (e.g.,
"env") — detect when baseValue or overrideValue is a slice vs map and convert
list syntax into the canonical map/list representation (or merge by name/append
deduplicated entries) before assigning merged[key]; ensure mergedRaw is produced
from the normalized merged structure so YAML syntax differences no longer affect
semantics (refer to mergeBaseDefinitionData, mergeDefinitionMaps, decode,
decodeDefinitionData).
Comment on lines
4258
to
4261
| loadOpts := []spec.LoadOption{ | ||
| spec.WithBaseConfig(a.config.Paths.BaseConfig), | ||
| spec.WithWorkspaceBaseConfigDir(workspace.BaseConfigDir(a.config.Paths.DAGsDir)), | ||
| spec.WithDAGsDir(a.config.Paths.DAGsDir), |
There was a problem hiding this comment.
Workspace base config is still skipped on the default reschedule path.
This only fixes the useCurrentDAGFile branch. The default branch still rebuilds from snapshotDAG.YamlData via loadInlineDAG on Line 3031, and that loader never reapplies the workspace layer or preserved snapshotDAG.BaseConfigData. As a result, rescheduling a named-workspace DAG still drops workspace-scoped settings unless callers explicitly opt into the current-file path.
🤖 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 `@internal/service/frontend/api/v1/dagruns.go` around lines 4258 - 4261, The
default reschedule path rebuilds the DAG using
loadInlineDAG(snapshotDAG.YamlData) without reapplying workspace or preserved
base-config layers, so when rescheduling a named-workspace DAG workspace-scoped
settings are lost; update the rebuild to pass the same spec.LoadOption values as
the useCurrentDAGFile branch (include
spec.WithWorkspaceBaseConfigDir(workspace.BaseConfigDir(...)) and
spec.WithBaseConfig populated from snapshotDAG.BaseConfigData or the persisted
base config) or change loadInlineDAG to accept and apply those load options and
honor snapshotDAG.BaseConfigData so the workspace layer and preserved
base-config are reapplied during the snapshot-based load.
Comment on lines
+33
to
+38
| // BaseConfigPath returns the workspace-scoped base config path for name. | ||
| func BaseConfigPath(dagsDir, name string) string { | ||
| if strings.TrimSpace(dagsDir) == "" || strings.TrimSpace(name) == "" { | ||
| return "" | ||
| } | ||
| return filepath.Join(BaseConfigDir(dagsDir), name, BaseConfigFileName) |
There was a problem hiding this comment.
Validate name before building the workspace config path.
filepath.Join(BaseConfigDir(dagsDir), name, BaseConfigFileName) will happily turn ../other into a path outside workspaces/, so any caller that forgets to validate the workspace name can read or write the wrong file. Since this helper already returns "" for invalid input, it should also reject names that fail ValidateName.
Suggested guard
func BaseConfigPath(dagsDir, name string) string {
if strings.TrimSpace(dagsDir) == "" || strings.TrimSpace(name) == "" {
return ""
}
+ if err := ValidateName(name); err != nil {
+ return ""
+ }
return filepath.Join(BaseConfigDir(dagsDir), name, BaseConfigFileName)
}🤖 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 `@internal/workspace/workspace.go` around lines 33 - 38, The BaseConfigPath
function currently builds a path with the raw name which allows names like
"../other" to escape the workspaces directory; call ValidateName(name) (or
equivalent workspace name validator) at the start of BaseConfigPath and return
"" if validation fails so only validated names are joined (i.e., add a guard: if
!ValidateName(name) return "") before calling BaseConfigDir and filepath.Join.
Comment on lines
+87
to
+90
| useEffect(() => { | ||
| setWorkspaceValue(null); | ||
| setWorkspaceDirty(false); | ||
| }, [selectedWorkspace, remoteNode]); |
There was a problem hiding this comment.
Reset global scope state on remoteNode change to avoid stale/cross-node writes.
The workspace scope correctly resets its local state when remoteNode (or selectedWorkspace) changes, but there is no equivalent reset for the global scope. Because the init effect at Lines 69-73 only seeds globalValue when it is null, switching remote nodes keeps the previous node's globalValue (and any globalDirty edits) in the editor even though globalData refetches against the new node. If the user had unsaved edits before switching, clicking Save will PUT the old node's content to the new node's /settings/base-config endpoint.
🛠️ Proposed fix
useEffect(() => {
setWorkspaceValue(null);
setWorkspaceDirty(false);
}, [selectedWorkspace, remoteNode]);
+
+ useEffect(() => {
+ setGlobalValue(null);
+ setGlobalDirty(false);
+ }, [remoteNode]);📝 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
| useEffect(() => { | |
| setWorkspaceValue(null); | |
| setWorkspaceDirty(false); | |
| }, [selectedWorkspace, remoteNode]); | |
| useEffect(() => { | |
| setWorkspaceValue(null); | |
| setWorkspaceDirty(false); | |
| }, [selectedWorkspace, remoteNode]); | |
| useEffect(() => { | |
| setGlobalValue(null); | |
| setGlobalDirty(false); | |
| }, [remoteNode]); |
🤖 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 `@ui/src/pages/base-config/index.tsx` around lines 87 - 90, The global scope
isn't reset when remoteNode (or selectedWorkspace) changes, leaving stale
globalValue/globalDirty and risking cross-node saves; add a useEffect that
listens to [selectedWorkspace, remoteNode] and calls setGlobalValue(null) and
setGlobalDirty(false) (mirroring the workspace reset) so globalValue will be
re-seeded from globalData for the new node; reference the existing state setters
setGlobalValue and setGlobalDirty and the globalData/init effect that seeds
globalValue when null.
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
Details
workspaces/<workspace>/base.yamlValidation
go test ./internal/core/spec ./internal/persis/filedag ./internal/gitsync ./internal/service/frontend/api/v1 ./internal/cmd ./internal/engine ./internal/service/frontend ./internal/test ./internal/workspace -count=1pnpm testpnpm typecheckpnpm buildgit diff --checkSummary by CodeRabbit
Release Notes
New Features
User Interface