fix(desktop): address template and team review findings

Preserve template channel settings and duplicated team metadata, validate team mention constraints before persistence, safely unlock failed section defaults, and document template folder APIs.

Co-authored-by: Fast Fizz <2df81cb51f05a9d5387ef24d7b9ecb8fcdfcd1c70ffabc67061c9596e1b5b1c4@buzz.block.builderlab.xyz>
Co-authored-by: Carl <3c4caeafb646d23867f1c4832e68211d77e2561946171625f75c3ce1a3f2670f@buzz.block.builderlab.xyz>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
This commit is contained in:
kenny lopez
2026-08-10 17:18:08 +01:00
committed by Carl
parent 60362872fd
commit 197ed0aaf0
17 changed files with 269 additions and 36 deletions

View File

@@ -72,6 +72,11 @@ fn validate_visibility(value: &str) -> Result<(), String> {
}
#[tauri::command]
/// Open a native multi-folder picker for channel-template project folders.
///
/// Returns an empty list when the user cancels. Returns an error if the dialog
/// closes unexpectedly or any selected folder cannot be represented as a local
/// filesystem path.
pub async fn pick_channel_template_project_folder(app: AppHandle) -> Result<Vec<String>, String> {
use tauri_plugin_dialog::DialogExt;

View File

@@ -160,10 +160,8 @@ pub(crate) fn build_import_team(
persona_ids: Vec<String>,
now: &str,
) -> Result<TeamRecord, String> {
let name = snapshot.team.name.trim();
if name.is_empty() {
return Err("Team snapshot name is empty.".to_string());
}
let name = crate::managed_agents::validate_team_name(&snapshot.team.name)
.map_err(|error| format!("Invalid team snapshot: {error}"))?;
Ok(TeamRecord {
id: Uuid::new_v4().to_string(),

View File

@@ -329,6 +329,17 @@ fn team_import_definitions_are_built_for_all_members() {
assert_eq!(definitions[0].system_prompt, "Alice prompt");
}
#[test]
fn team_import_rejects_names_that_cannot_be_mentioned() {
for invalid_name in ["x".repeat(201), "Review\nTeam".to_string()] {
let mut source = snapshot(vec![member("Alice")]);
source.team.name = invalid_name;
let error = build_import_team(&source, vec!["alice".to_string()], "now").unwrap_err();
assert!(error.starts_with("Invalid team snapshot: Team name"));
}
}
#[test]
fn team_import_keeps_or_clears_every_member_allowlist_with_one_toggle() {
let source = snapshot(vec![member("Alice"), member("Bob")]);

View File

@@ -5,19 +5,12 @@ use crate::{
app_state::AppState,
managed_agents::{
delete_team_with_cascade, ensure_persona_ids_are_active, load_personas, load_teams,
save_teams, try_regenerate_nest, CreateTeamRequest, TeamRecord, UpdateTeamRequest,
save_teams, try_regenerate_nest, validate_team_name, CreateTeamRequest, TeamRecord,
UpdateTeamRequest,
},
util::now_iso,
};
fn trim_required(value: &str, label: &str) -> Result<String, String> {
let trimmed = value.trim();
if trimmed.is_empty() {
return Err(format!("{label} is required"));
}
Ok(trimmed.to_string())
}
fn trim_optional(value: Option<String>) -> Option<String> {
value.and_then(|candidate| {
let trimmed = candidate.trim();
@@ -145,7 +138,7 @@ pub async fn create_team(input: CreateTeamRequest, app: AppHandle) -> Result<Tea
use tauri::Manager;
tokio::task::spawn_blocking(move || {
let state = app.state::<AppState>();
let name = trim_required(&input.name, "Team name")?;
let name = validate_team_name(&input.name)?;
let description = trim_optional(input.description);
let instructions = trim_optional(input.instructions);
let now = now_iso();
@@ -186,7 +179,7 @@ pub async fn update_team(input: UpdateTeamRequest, app: AppHandle) -> Result<Tea
use tauri::Manager;
tokio::task::spawn_blocking(move || {
let state = app.state::<AppState>();
let name = trim_required(&input.name, "Team name")?;
let name = validate_team_name(&input.name)?;
let description = trim_optional(input.description);
let instructions = trim_optional(input.instructions);

View File

@@ -1,6 +1,7 @@
use crate::managed_agents::MAX_TEAM_NAME_CHARS;
use nostr::Tag;
const MAX_TEAM_MENTION_FIELD_CHARS: usize = 200;
const MAX_TEAM_MENTION_ID_CHARS: usize = 200;
pub(super) fn parse_mention_reference_tag(mention: &[String]) -> Result<Tag, String> {
match mention.first().map(String::as_str) {
@@ -40,8 +41,8 @@ fn parse_team_mention(mention: &[String]) -> Result<Tag, String> {
if team_id.is_empty() || display_name.is_empty() {
return Err("team mention id and display name must not be blank".into());
}
if team_id.chars().count() > MAX_TEAM_MENTION_FIELD_CHARS
|| display_name.chars().count() > MAX_TEAM_MENTION_FIELD_CHARS
if team_id.chars().count() > MAX_TEAM_MENTION_ID_CHARS
|| display_name.chars().count() > MAX_TEAM_NAME_CHARS
{
return Err("team mention id and display name must be at most 200 characters".into());
}

View File

@@ -206,9 +206,8 @@ pub(crate) fn validate_team_snapshot(snapshot: &TeamSnapshot) -> Result<(), Stri
snapshot.version
));
}
if snapshot.team.name.trim().is_empty() {
return Err("Team snapshot team.name is empty".to_string());
}
crate::managed_agents::validate_team_name(&snapshot.team.name)
.map_err(|error| format!("Invalid team snapshot: {error}"))?;
if snapshot.members.is_empty() {
return Err("Team snapshot must have at least one member".to_string());
}
@@ -423,7 +422,18 @@ mod tests {
let bytes = serde_json::to_vec(&snapshot).unwrap();
let result = decode_team_snapshot_json(&bytes);
assert!(result.is_err());
assert!(result.unwrap_err().contains("team.name is empty"));
assert!(result.unwrap_err().contains("Team name is required"));
}
#[test]
fn validate_rejects_team_name_that_cannot_be_mentioned() {
for invalid_name in ["x".repeat(201), "Review\nTeam".to_string()] {
let mut snapshot = two_member_team();
snapshot.team.name = invalid_name;
let bytes = serde_json::to_vec(&snapshot).unwrap();
let error = decode_team_snapshot_json(&bytes).unwrap_err();
assert!(error.starts_with("Invalid team snapshot: Team name"));
}
}
#[test]

View File

@@ -9,6 +9,27 @@ use crate::{
use super::team_repair::team_persona_key;
/// Maximum team-name length accepted by the `team_mention` event tag.
pub const MAX_TEAM_NAME_CHARS: usize = 200;
/// Normalize and validate a team name before it can reach storage or event
/// construction.
pub fn validate_team_name(value: &str) -> Result<String, String> {
let name = value.trim();
if name.is_empty() {
return Err("Team name is required".to_string());
}
if name.chars().count() > MAX_TEAM_NAME_CHARS {
return Err(format!(
"Team name must be at most {MAX_TEAM_NAME_CHARS} characters"
));
}
if name.chars().any(char::is_control) {
return Err("Team name must not contain control characters".to_string());
}
Ok(name.to_string())
}
pub(crate) fn teams_store_path(app: &AppHandle) -> Result<PathBuf, String> {
Ok(managed_agents_base_dir(app)?.join("teams.json"))
}

View File

@@ -5,7 +5,7 @@
use super::{
agents_referencing_team, load_teams_readonly, merge_teams, merge_teams_impl, sort_teams,
validate_team_deletion, BuiltInTeam,
validate_team_deletion, validate_team_name, BuiltInTeam, MAX_TEAM_NAME_CHARS,
};
use crate::managed_agents::{ManagedAgentRecord, TeamRecord};
@@ -26,6 +26,31 @@ fn team(id: &str, name: &str) -> TeamRecord {
}
}
#[test]
fn validate_team_name_normalizes_valid_names() {
assert_eq!(
validate_team_name(" Review Team ").unwrap(),
"Review Team"
);
assert!(validate_team_name(&"é".repeat(MAX_TEAM_NAME_CHARS)).is_ok());
}
#[test]
fn validate_team_name_rejects_unmentionable_names() {
assert_eq!(
validate_team_name(" \t ").unwrap_err(),
"Team name is required"
);
assert_eq!(
validate_team_name(&"x".repeat(MAX_TEAM_NAME_CHARS + 1)).unwrap_err(),
"Team name must be at most 200 characters"
);
assert_eq!(
validate_team_name("Review\nTeam").unwrap_err(),
"Team name must not contain control characters"
);
}
#[test]
fn sort_teams_alphabetical_case_insensitive() {
let mut teams = vec![team("3", "Zulu"), team("1", "alpha"), team("2", "Bravo")];

View File

@@ -38,8 +38,13 @@ pub struct TemplateAgentRoster {
#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
/// Workspace settings applied when creating a worktree from a channel template.
/// `location` is the parent directory for new worktrees; `base_branch` is the
/// branch fetched from origin and used as each worktree's starting point.
pub struct TemplateWorktreeConfig {
/// Parent directory where new worktrees are created.
pub location: String,
/// Branch used as the base for each new worktree.
pub base_branch: String,
}

View File

@@ -144,7 +144,11 @@ export function TeamDialog({
...baseInput,
};
}
return baseInput;
return {
...baseInput,
description: teamDescription.trim() || undefined,
instructions: instructions.trim() || undefined,
};
}
async function handleSubmit() {

View File

@@ -0,0 +1,28 @@
import assert from "node:assert/strict";
import test from "node:test";
import {
exceedsMentionPubkeyLimit,
MAX_MENTION_PUBKEYS,
} from "./useMentionSendFlow.helpers.ts";
test("mention preflight accepts the maximum number of unique targets", () => {
const pubkeys = Array.from({ length: MAX_MENTION_PUBKEYS }, (_, index) =>
index.toString(16).padStart(64, "0"),
);
assert.equal(exceedsMentionPubkeyLimit(pubkeys, []), false);
assert.equal(exceedsMentionPubkeyLimit([...pubkeys, pubkeys[0]], []), false);
});
test("mention preflight counts unresolved persona targets before provisioning", () => {
const pubkeys = Array.from({ length: MAX_MENTION_PUBKEYS - 1 }, (_, index) =>
index.toString(16).padStart(64, "0"),
);
assert.equal(exceedsMentionPubkeyLimit(pubkeys, ["persona-1"]), false);
assert.equal(
exceedsMentionPubkeyLimit(pubkeys, ["persona-1", "persona-2"]),
true,
);
});

View File

@@ -81,6 +81,21 @@ export function uniqueNormalizedPubkeys(pubkeys: Iterable<string>) {
return [...new Set([...pubkeys].map(normalizePubkey))].filter(Boolean);
}
export const MAX_MENTION_PUBKEYS = 50;
export function exceedsMentionPubkeyLimit(
resolvedPubkeys: Iterable<string>,
unresolvedPersonaIds: Iterable<string>,
) {
const targetPubkeys = new Set(
[...resolvedPubkeys].map(normalizePubkey).filter(Boolean),
);
for (const personaId of unresolvedPersonaIds) {
if (personaId.trim()) targetPubkeys.add(`persona:${personaId}`);
}
return targetPubkeys.size > MAX_MENTION_PUBKEYS;
}
export function isManagedAgentRunning(agent: ManagedAgent) {
return agent.status === "running" || agent.status === "deployed";
}

View File

@@ -31,6 +31,7 @@ import type { AcpRuntime, ManagedAgent } from "@/shared/api/types";
import { normalizePubkey, truncatePubkey } from "@/shared/lib/pubkey";
import {
buildMentionTags,
exceedsMentionPubkeyLimit,
getErrorMessage,
isManagedAgentRunning,
isProviderBackedAgent,
@@ -683,6 +684,23 @@ export function useMentionSendFlow({
return;
}
const preflightMentionPubkeys = mentions.extractMentionPubkeys(trimmed);
const preflightPersonaIds = mentions
.extractMentionPersonas(trimmed)
.map(({ persona }) => persona.id);
if (
exceedsMentionPubkeyLimit(
preflightMentionPubkeys,
preflightPersonaIds,
)
) {
const message =
"Too many mentions. Messages can mention at most 50 people or agents.";
setNonMemberPromptError(message);
toast.error(message);
return;
}
let effectiveChannelId = capturedChannelId;
if (!effectiveChannelId && onPrepareSendChannel) {
effectiveChannelId = await onPrepareSendChannel();
@@ -798,6 +816,7 @@ export function useMentionSendFlow({
mentions.isManagedAgentPubkey,
mentions.getDraftMentionRefs,
onPrepareSendChannel,
mentions.extractMentionPersonas,
],
);

View File

@@ -496,6 +496,12 @@ export function TemplateFormDialog({
const input: UpdateChannelTemplateInput = {
id: template.id,
name: trimmedName,
// The current editor intentionally does not expose channel type or
// visibility, so preserve the stored values when updating unrelated
// template fields rather than letting the backend's defaults overwrite
// them.
channelType: template.channelType,
visibility: template.visibility,
description: template.description ?? undefined,
canvasTemplate: canvasTemplate.trim() || undefined,
projectFolders,

View File

@@ -0,0 +1,42 @@
import assert from "node:assert/strict";
import test from "node:test";
import { resolveInitialChannelTemplate } from "./useCreateChannelForm.ts";
const template = {
id: "review-template",
name: "Review",
description: "Review changes",
channelType: "stream",
visibility: "private",
canvasTemplate: null,
projectFolders: [],
projectFolder: null,
worktree: null,
agents: { personas: [], teams: [] },
isBuiltin: false,
createdAt: "now",
updatedAt: "now",
};
test("keeps a section template locked while the template query is pending", () => {
assert.equal(
resolveInitialChannelTemplate(template.id, [], "pending"),
undefined,
);
});
test("applies a section template after a successful query", () => {
assert.equal(
resolveInitialChannelTemplate(template.id, [template], "success"),
template,
);
});
test("clears a missing section template after a successful query", () => {
assert.equal(resolveInitialChannelTemplate(template.id, [], "success"), null);
});
test("clears a locked section template after a terminal query failure", () => {
assert.equal(resolveInitialChannelTemplate(template.id, [], "error"), null);
});

View File

@@ -4,6 +4,26 @@ import { useChannelTemplatesQuery } from "@/features/channel-templates/hooks";
import { DEFAULT_EPHEMERAL_TTL_SECONDS } from "@/features/channels/lib/ephemeralChannel";
import type { ChannelTemplate, ChannelVisibility } from "@/shared/api/types";
export type TemplateQueryStatus = "pending" | "success" | "error";
/**
* Resolve a section's saved template against the template query.
*
* `undefined` means the query is still pending, `null` means the saved
* template must be cleared, and a template means the default can be applied.
*/
export function resolveInitialChannelTemplate(
initialTemplateId: string | undefined,
templates: ChannelTemplate[],
status: TemplateQueryStatus,
): ChannelTemplate | null | undefined {
if (!initialTemplateId || status === "pending") return undefined;
if (status === "error") return null;
return (
templates.find((template) => template.id === initialTemplateId) ?? null
);
}
export type CreateChannelKind = "stream" | "forum";
export type CreateChannelInput = {
@@ -82,6 +102,8 @@ export function useCreateChannelForm({
const [selectedTemplateId, setSelectedTemplateId] = React.useState<
string | null
>(initialTemplateId ?? null);
const [sectionDefaultTemplateId, setSectionDefaultTemplateId] =
React.useState<string | null>(initialTemplateId ?? null);
const [typePopoverOpen, setTypePopoverOpen] = React.useState(false);
const nameInputRef = React.useRef<HTMLInputElement>(null);
const visibilityTouchedRef = React.useRef(false);
@@ -101,6 +123,7 @@ export function useCreateChannelForm({
setTtlSeconds(DEFAULT_EPHEMERAL_TTL_SECONDS);
setErrorMessage(null);
setSelectedTemplateId(initialTemplateId ?? null);
setSectionDefaultTemplateId(initialTemplateId ?? null);
setTypePopoverOpen(false);
visibilityTouchedRef.current = false;
initialTemplateAppliedRef.current = null;
@@ -134,20 +157,42 @@ export function useCreateChannelForm({
}, []);
React.useEffect(() => {
if (
!active ||
!initialTemplateId ||
initialTemplateAppliedRef.current === initialTemplateId
) {
if (!active || !initialTemplateId) return;
const queryStatus: TemplateQueryStatus = templatesQuery.isError
? "error"
: templatesQuery.isSuccess
? "success"
: "pending";
const template = resolveInitialChannelTemplate(
initialTemplateId,
templates,
queryStatus,
);
if (template === undefined) return;
if (template === null) {
// Missing templates and terminal query failures both unlock the form.
// Keeping the raw section ID selected would promise defaults that cannot
// be applied after channel creation.
initialTemplateAppliedRef.current = initialTemplateId;
setSelectedTemplateId(null);
setSectionDefaultTemplateId(null);
setDescription("");
if (!visibilityTouchedRef.current) setVisibility("open");
setErrorMessage(null);
return;
}
const template = templates.find(
(candidate) => candidate.id === initialTemplateId,
);
if (!template) return;
if (initialTemplateAppliedRef.current === initialTemplateId) return;
initialTemplateAppliedRef.current = initialTemplateId;
applyTemplate(template);
}, [active, applyTemplate, initialTemplateId, templates]);
}, [
active,
applyTemplate,
initialTemplateId,
templates,
templatesQuery.isError,
templatesQuery.isSuccess,
]);
const handleTemplateCreated = React.useCallback(
(template: ChannelTemplate) => {
@@ -246,7 +291,7 @@ export function useCreateChannelForm({
handleTemplateChange,
handleTemplateCreated,
templates,
sectionDefaultTemplateId: initialTemplateId ?? null,
sectionDefaultTemplateId,
nameInputRef,
isCreating,
canSubmit: name.trim().length > 0 && !isCreating,

View File

@@ -17,7 +17,12 @@ export type TemplateTeamEntry = {
backend: TemplateBackend | null;
};
export type TemplateWorktreeConfig = { location: string; baseBranch: string };
export type TemplateWorktreeConfig = {
/** Directory where new worktrees are created. */
location: string;
/** Branch used as the base for each new worktree. */
baseBranch: string;
};
export type TemplateAgents = {
personas: TemplateAgentEntry[];
teams: TemplateTeamEntry[];