feat(setup): add dashboard setup entry#693
Conversation
|
Review posted through GitHub's PR review UI for commit |
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
261824c6
I did not find a concrete correctness, security, data integrity, or breaking-change issue in the changed setup and pipeline settings paths. The implementation matches the stated direction: dashboard setup creates defaults and opens the dashboard for fresh setup, and the dashboard helper now writes only memory.pipelineV2 provider settings rather than dashboard-specific inference routing.
Confidence: Medium [sufficient_diff_evidence, targeted_context_included, missing_runtime_repro] - The relevant changed files and tests are included, and the new dashboard setup branch plus provider-only pipeline helper are directly visible in the diff. I did not run the setup flow or dashboard UI, so this does not cover runtime prompt behavior or visual regressions beyond what the code and tests show.
Note: This PR touches UI files but no screenshots were referenced in the description. Consider adding visual previews for reviewers.
261824c to
ab9e1f3
Compare
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
da29f110
I found one dashboard onboarding state bug: it can silently re-enable session synthesis even when the existing config explicitly disabled it.
Confidence: Medium [sufficient_diff_evidence, targeted_context_included, missing_runtime_repro] - The changed onboarding component hydrates provider/model from memory.pipelineV2 but derives synthesisEnabled only from provider !== "none". I did not run the Svelte UI, but the config write path in finish() passes that derived value back into applyRecommendedPipelineSetup, so the behavior is visible from the diff.
| selectedHarnesses = st.harnessArray(); | ||
| if (selectedHarnesses.length === 0 && harnessOptions.length > 0) selectedHarnesses = [harnessOptions[0].id]; | ||
| if (!selectedHarness && selectedHarnesses.length > 0) selectedHarness = selectedHarnesses[0]; | ||
| synthesisEnabled = provider !== "none"; |
There was a problem hiding this comment.
This drops an explicit memory.pipelineV2.synthesis.enabled: false when onboarding hydrates existing settings. If a fresh/defaultish workspace has extraction configured but synthesis intentionally disabled, the modal will show the toggle enabled and finish() writes that value back via applyRecommendedPipelineSetup, silently re-enabling synthesis. Please hydrate from the existing synthesis enabled value when present and only default to provider !== "none" when it is unset.
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
94ed81e9
The latest changes appear to be mostly visual polish, but the onboarding state bug from the prior review is still present: opening and saving the modal can re-enable synthesis that was explicitly disabled.
Confidence: Medium [sufficient_diff_evidence, targeted_context_included, missing_runtime_repro] - The included file contents show hydrateFromSettings still sets synthesisEnabled from provider !== "none", and finish still writes that value through applyRecommendedPipelineSetup. I did not run the Svelte UI, so this is based on the visible config hydration and save path rather than a browser reproduction.
Since my last review:
- Fixed or likely addressed: 1
- Still blocking: 0
- Newly found: 1
- Suppressed due to human rebuttal/out-of-scope: 0
| selectedHarnesses = st.harnessArray(); | ||
| if (selectedHarnesses.length === 0 && harnessOptions.length > 0) selectedHarnesses = [harnessOptions[0].id]; | ||
| if (!selectedHarness && selectedHarnesses.length > 0) selectedHarness = selectedHarnesses[0]; | ||
| synthesisEnabled = provider !== "none"; |
There was a problem hiding this comment.
This still ignores an explicit memory.pipelineV2.synthesis.enabled: false when the onboarding modal hydrates existing settings. For a defaultish workspace with extraction configured but synthesis intentionally disabled, opening the modal and clicking Save will pass synthesisEnabled: true into applyRecommendedPipelineSetup, silently re-enabling summary synthesis. Please hydrate this from the existing synthesis config, falling back to provider !== "none" only when there is no explicit setting.
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
7eb8e400
I still see one onboarding config-preservation bug: saving the modal can re-enable session synthesis even when the existing config explicitly disabled it. No new security issue jumped out from the changed setup or provider-settings paths.
Confidence: Medium [sufficient_diff_evidence, targeted_context_included, missing_runtime_repro] - The included OnboardingModal.svelte content still hydrates synthesisEnabled from provider !== "none" and finish() writes that value through applyRecommendedPipelineSetup. I did not run the Svelte modal, so this is based on the visible hydrate/save path rather than a browser reproduction.
Since my last review:
- Fixed or likely addressed: 1
- Still blocking: 0
- Newly found: 1
- Suppressed due to human rebuttal/out-of-scope: 0
| selectedHarnesses = st.harnessArray(); | ||
| if (selectedHarnesses.length === 0 && harnessOptions.length > 0) selectedHarnesses = [harnessOptions[0].id]; | ||
| if (!selectedHarness && selectedHarnesses.length > 0) selectedHarness = selectedHarnesses[0]; | ||
| synthesisEnabled = provider !== "none"; |
There was a problem hiding this comment.
This still ignores an explicit memory.pipelineV2.synthesis.enabled: false when the onboarding modal hydrates existing settings. For a defaultish workspace with extraction configured but synthesis intentionally disabled, just opening onboarding and saving will pass synthesisEnabled: true into applyRecommendedPipelineSetup, silently re-enabling background synthesis calls. Please hydrate this from the existing synthesis config, falling back to provider !== "none" only when it is unset.
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
ef2183a8
I still see one config-preservation bug in the onboarding modal: saving can re-enable synthesis that the user explicitly disabled. I did not find a new blocker in the latest diff.
Confidence: Medium [sufficient_diff_evidence, targeted_context_included, missing_runtime_repro] - The current OnboardingModal.svelte still hydrates synthesisEnabled from provider !== "none" and finish() writes that value back through applyRecommendedPipelineSetup. I did not run the Svelte modal, so this is based on the visible hydrate/save path rather than a browser reproduction.
Since my last review:
- Fixed or likely addressed: 1
- Still blocking: 0
- Newly found: 1
- Suppressed due to human rebuttal/out-of-scope: 0
| selectedHarnesses = st.harnessArray(); | ||
| if (selectedHarnesses.length === 0 && harnessOptions.length > 0) selectedHarnesses = [harnessOptions[0].id]; | ||
| if (!selectedHarness && selectedHarnesses.length > 0) selectedHarness = selectedHarnesses[0]; | ||
| synthesisEnabled = provider !== "none"; |
There was a problem hiding this comment.
This still ignores an explicit memory.pipelineV2.synthesis.enabled: false when onboarding hydrates existing settings. For a defaultish workspace with extraction configured but synthesis intentionally disabled, opening the modal and saving will set synthesisEnabled = true and write synthesis back on via applyRecommendedPipelineSetup(). Please hydrate this from the existing synthesis config, falling back to provider !== "none" only when no explicit synthesis enabled value exists.
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
0bc63fd0
I still see one config-preservation bug in the onboarding modal: saving can re-enable session synthesis that the user explicitly disabled. I did not find a new blocker in the latest changes.
Confidence: Medium [sufficient_diff_evidence, targeted_context_included, missing_runtime_repro] - The current OnboardingModal.svelte still hydrates synthesisEnabled from provider !== "none" and finish() writes that value back through applyRecommendedPipelineSetup. I did not run the Svelte modal, so this is based on the visible hydrate/save path rather than a browser reproduction.
Since my last review:
- Fixed or likely addressed: 1
- Still blocking: 0
- Newly found: 1
- Suppressed due to human rebuttal/out-of-scope: 0
| selectedHarnesses = st.harnessArray(); | ||
| if (selectedHarnesses.length === 0 && harnessOptions.length > 0) selectedHarnesses = [harnessOptions[0].id]; | ||
| if (!selectedHarness && selectedHarnesses.length > 0) selectedHarness = selectedHarnesses[0]; | ||
| synthesisEnabled = provider !== "none"; |
There was a problem hiding this comment.
This still ignores an explicit memory.pipelineV2.synthesis.enabled: false when the onboarding modal hydrates existing settings. For a defaultish workspace with extraction configured but synthesis intentionally disabled, opening and saving onboarding will set synthesisEnabled to true here and then finish() writes that back through applyRecommendedPipelineSetup, re-enabling synthesis. This should hydrate from the existing synthesis config instead of deriving it only from the extraction provider.
There was a problem hiding this comment.
Fixed in 322ad033c: onboarding now hydrates the toggle with resolveSynthesisEnabled(st.agent) instead of deriving it from the extraction provider, so an explicit memory.pipelineV2.synthesis.enabled: false is preserved when saving.
Validation:
bun test surfaces/dashboard/src/lib/components/tabs/settings/pipeline-settings.test.tscd surfaces/dashboard && bun run check(0 errors; existing unrelated warnings)cd surfaces/dashboard && bun run build(passes; existing unrelated warnings)
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
322ad033
The previously flagged synthesis-preservation issue appears addressed: onboarding now hydrates the toggle through resolveSynthesisEnabled, which preserves an explicit disabled synthesis block. I did not find a new correctness, security, or data-integrity issue in the current diff.
Confidence: Medium [sufficient_diff_evidence, targeted_context_included, missing_runtime_repro] - The incremental diff directly changes OnboardingModal.svelte to use resolveSynthesisEnabled(st.agent), and the helper returns an explicit memory.pipelineV2.synthesis.enabled boolean when present. I did not run the dashboard modal or CLI setup path, so this is based on the provided diff and changed-file context.
Since my last review:
- Fixed or likely addressed: 1
- Still blocking: 0
- Newly found: 0
- Suppressed due to human rebuttal/out-of-scope: 0
Rebase onto main required resolving conflicts in pipeline-settings, PipelineSection, and test files. Updated function signatures to merge acpx dashboard agent type with extended options, fixed unused imports, and aligned test expectations with the updated function output.
322ad03 to
9156644
Compare
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
9156644f
I found two issues that should be fixed before this lands: the dashboard setup path appears to build an incomplete FreshSetupConfig, and the shared dashboard helper still writes ACPX inference routing even when the PR says it now only writes pipeline provider settings.
Confidence: Medium [sufficient_diff_evidence, targeted_context_included, missing_runtime_repro] - The findings are directly visible in the changed setup and pipeline-settings code. I did not have a runnable checkout for typecheck/runtime reproduction, so the FreshSetupConfig issue is based on the provided changed file contents and interface shape.
Note: This PR touches UI files but no screenshots were referenced in the description. Consider adding visual previews for reviewers.
Since my last review:
- Fixed or likely addressed: 0
- Still blocking: 0
- Newly found: 2
- Suppressed due to human rebuttal/out-of-scope: 0
| if (setupMethod === "github") { | ||
| mkdirSync(basePath, { recursive: true }); | ||
| mkdirSync(join(basePath, "memory"), { recursive: true }); | ||
| await deps.importFromGitHub(basePath); |
There was a problem hiding this comment.
This new dashboard setup branch calls runFreshSetup without the required identityPreset, startupIdentityFiles, and specialIdentityFiles fields from FreshSetupConfig. The normal setup path computes those later, but this early return bypasses that block. Unless the real branch has made those fields optional elsewhere, signet setup --setup-mode dashboard will fail typecheck/build or hand runFreshSetup an incomplete config.
| @@ -200,7 +229,7 @@ export function applyAcpxDashboardSetup( | |||
| targets["background-acpx"] = { | |||
There was a problem hiding this comment.
The PR description and new settings copy say the dashboard setup uses memory.pipelineV2 provider settings instead of dashboard-specific ACPX routing, but this helper still unconditionally writes inference.targets['background-acpx'], policies, task classes, and workloads. That also happens for non-ACPX providers like llama-cpp or ollama, so using the recommended setup can leave stale/contradictory ACPX inference routing in the config while the UI claims it only wrote pipeline provider settings.
Replace the monolithic onboarding modal with a multi-step wizard covering identity presets, agent naming/harnesses, embedding config, extraction pipeline, and a review step. Step content now lives in focused components with smooth fly transitions, a fixed-size modal, and progress dots in the header. Shared state is managed through onboarding-state.svelte.ts.
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
a2036a7c
I found two correctness problems in the new dashboard setup path: the CLI dashboard mode appears to omit required fresh-setup identity fields, and the onboarding embedding step does not actually persist the embedding choices it collects.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - The missing fresh setup fields are visible in the provided setup.ts call site versus the provided FreshSetupConfig interface, and the dashboard finish handler explicitly writes identity, harnesses, and pipeline extraction settings but never writes the selected embedding provider/model/endpoint.
Note: This PR touches UI files but no screenshots were referenced in the description. Consider adding visual previews for reviewers.
Since my last review:
- Fixed or likely addressed: 2
- Still blocking: 0
- Newly found: 3
- Suppressed due to human rebuttal/out-of-scope: 0
| await deps.importFromGitHub(basePath); | ||
| return; | ||
| } | ||
| if (setupMethod === "dashboard") { |
There was a problem hiding this comment.
This dashboard-mode runFreshSetup config is missing the required identity fields that the fresh setup path now expects: identityPreset, startupIdentityFiles, and specialIdentityFiles. The normal fresh setup path computes and passes those later, and FreshSetupConfig marks them required. As written, signet setup --setup-mode dashboard either fails typecheck/build or reaches runFreshSetup without the identity preset data needed to create the safe defaults the PR description promises.
| if (obState.saving) return; | ||
| obState.saving = true; | ||
| try { | ||
| st.aSetStr(["agent", "name"], obState.agentName.trim() || "My Agent"); |
There was a problem hiding this comment.
The onboarding flow asks the user to configure embeddings, but finish() never writes obState.embeddingProvider, embeddingModel, or embeddingEndpoint into st.agent. It only persists identity/harnesses and then calls applyRecommendedPipelineSetup, which updates extraction/synthesis under memory.pipelineV2. Changing embeddings to Ollama/OpenAI/none in the modal is therefore silently lost when saving.
| if (!obState.embeddingModel.trim()) return ["Embedding model is required."]; | ||
| return []; | ||
| } | ||
| if (step === 3) { |
There was a problem hiding this comment.
The extraction step has endpoint validation in its component, but the modal's actual validateStep() path only checks that the extraction model is non-empty. That means a user can enter a malformed local/API endpoint, continue, and save it into memory.pipelineV2. The CLI path validates extraction endpoints, so the dashboard path should enforce the same boundary before persisting config.
Summary
signet setup, including--setup-mode dashboard, so fresh installs can create safe defaults and open the dashboard instead of walking the full terminal wizard.memory.pipelineV2extraction/synthesis provider settings instead of inventing dashboard-specific ACPX routing.Test Plan
bun run build:corebun test surfaces/dashboard/src/lib/components/tabs/settings/pipeline-settings.test.ts surfaces/cli/src/commands/app.test.tsbunx biome check surfaces/cli/src/commands/app.ts surfaces/cli/src/commands/app.test.ts surfaces/cli/src/features/setup.ts surfaces/cli/src/features/setup-types.ts surfaces/dashboard/src/lib/components/tabs/settings/pipeline-settings.ts surfaces/dashboard/src/lib/components/tabs/settings/pipeline-settings.test.tsbun run --filter '@signet/cli' build:clicd surfaces/dashboard && bun run check(passes with existing unrelated warnings)PR Readiness (MANDATORY)
INDEX.md+dependencies.yaml)Migration Notes