Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-branch-name-extra-slash.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"tinacms": patch
---

Fix "Save to new branch" failing with "Branch operation failed" when the derived branch name is not a valid Git ref, e.g. when a collection's `path` has a trailing slash, producing `content/articles//foo.mdx` and the invalid ref `tina/articles//foo`. The default branch name derived from the file path, and any user-typed name, are now normalised to a valid ref: repeated and leading/trailing slashes collapse, characters Git forbids in refs (whitespace, control characters, `~ ^ : ? * [ \` and the `@{` sequence) become hyphens, `..` runs collapse, and leading dots and trailing `.` / `.lock` are stripped per path component. Saving is disabled while the name normalises to an empty string. The same normalisation now runs when creating a branch from the branch switcher and from the deleted-branch recovery modal, and the duplicated `formatBranchName` helpers are unified into a single util (the legacy branch switcher previously deleted invalid characters; it now replaces them with hyphens like the main switcher).
107 changes: 107 additions & 0 deletions .github/scripts/check-changeset.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
// Flags a PR that changes a published package's dependencies with no changeset: it would
// merge with no version bump and no release, so the change never reaches npm. Fails open.
import { execSync } from 'node:child_process';
import { readFileSync, existsSync, readdirSync, statSync } from 'node:fs';
import { join } from 'node:path';

const base = process.argv[2] ?? 'origin/main';

const git = (cmd) => execSync(cmd, { encoding: 'utf8', stdio: ['pipe', 'pipe', 'pipe'] });

const skip = (why) => {
console.log(`Changeset check skipped: ${why}`);
process.exit(0);
};

let changed;
try {
changed = git(`git diff --name-only ${base}...HEAD`).split('\n').filter(Boolean);
} catch {
skip(`cannot diff against "${base}". A shallow clone needs fetch-depth: 0.`);
}

if (changed.some((f) => /^\.changeset\/.+\.md$/.test(f))) {
console.log('A changeset is present. Nothing to check.');
process.exit(0);
}

let ignore = [];
try {
ignore = JSON.parse(readFileSync('.changeset/config.json', 'utf8')).ignore ?? [];
} catch {
skip('.changeset/config.json is missing or unreadable.');
}
const ignored = (name) =>
ignore.some((p) => (p.endsWith('/*') ? name.startsWith(p.slice(0, -1)) : p === name));

// Walked rather than a fixed directory list, so a new nesting level cannot drop out silently.
const published = new Map();
const walk = (dir, depth = 0) => {
if (depth > 4 || !existsSync(dir)) return;
const manifest = join(dir, 'package.json');
if (existsSync(manifest)) {
try {
const pkg = JSON.parse(readFileSync(manifest, 'utf8'));
if (!pkg.private && pkg.name && !ignored(pkg.name)) {
published.set(pkg.name, { manifest, pkg });
}
return; // a package root is a leaf; do not descend into its own subpackages
} catch {
/* unparseable manifest: ignore rather than fail the PR */
}
}
for (const entry of readdirSync(dir)) {
if (entry === 'node_modules' || entry.startsWith('.')) continue;
const next = join(dir, entry);
if (statSync(next).isDirectory()) walk(next, depth + 1);
}
};
walk('packages');

const affected = new Set();

for (const [name, { manifest }] of published) {
if (changed.includes(manifest)) affected.add(name);
}

if (changed.includes('pnpm-workspace.yaml')) {
const catalogKeys = (text) => {
const out = new Map();
let inCatalog = false;
for (const line of text.split('\n')) {
if (/^catalog:\s*$/.test(line)) { inCatalog = true; continue; }
if (inCatalog && /^\S/.test(line)) break;
const m = inCatalog && line.match(/^\s+(\S+):\s*(.+?)\s*$/);
if (m) out.set(m[1].replace(/^['"]|['"]$/g, ''), m[2]);
}
return out;
};

let before;
try {
before = catalogKeys(git(`git show ${base}:pnpm-workspace.yaml`));
} catch {
before = new Map(); // absent at base: treat every current entry as new
}
const after = catalogKeys(readFileSync('pnpm-workspace.yaml', 'utf8'));
const moved = [...after].filter(([k, v]) => before.get(k) !== v).map(([k]) => k);

for (const [name, { pkg }] of published) {
const deps = { ...pkg.dependencies, ...pkg.devDependencies, ...pkg.peerDependencies };
if (moved.some((dep) => deps[dep]?.startsWith('catalog:'))) affected.add(name);
}
}

if (affected.size === 0) {
console.log('No published package is affected. No changeset required.');
process.exit(0);
}

console.error(
`No changeset found, but this PR changes dependencies of published packages:\n` +
[...affected].sort().map((n) => ` - ${n}`).join('\n') +
`\n\nWithout a changeset these packages get no version bump and no release, so the\n` +
`change never reaches npm users. Run "pnpm changeset" and commit the result.\n` +
`If the change genuinely needs no release, add an empty changeset ("pnpm changeset --empty").`,
);
process.exit(1);
29 changes: 29 additions & 0 deletions .github/workflows/require-changeset.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
name: Require Changeset

on:
pull_request:
types: [opened, synchronize, reopened, labeled]

permissions:
contents: read

jobs:
require-changeset:
runs-on: ubuntu-latest
timeout-minutes: 5
# Escape hatch for a change that genuinely ships nothing. Dependabot PRs are NOT
# exempt: they are the ones that keep merging unreleased (tinacms/tinacms#7435).
if: "!contains(github.event.pull_request.labels.*.name, 'skip-changeset')"
steps:
- name: Check out code
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
with:
fetch-depth: 0

- name: Set up Node
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
with:
node-version-file: .nvmrc

- name: Require a changeset for published packages
run: node .github/scripts/check-changeset.mjs origin/${{ github.base_ref }}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
ModalHeader,
PopupModal,
} from '@toolkit/react-modals';
import { normalizeBranchName } from '@utils/branch-name';
import { CircleAlert } from 'lucide-react';
import * as React from 'react';

Expand Down Expand Up @@ -116,7 +117,7 @@ export const MediaWorkflowOverlay = () => {

const confirmState = state;
const branchName = confirmState.branchName;
const targetBranch = `tina/${branchName}`;
const targetBranch = `tina/${normalizeBranchName(branchName)}`;
abortPreflight();
const abortController = new AbortController();
preflightAbortRef.current = abortController;
Expand Down Expand Up @@ -190,7 +191,9 @@ export const MediaWorkflowOverlay = () => {
state.onCancel();
setState({ phase: 'idle' });
}}
disabled={state.branchName === '' || state.isChecking}
disabled={
normalizeBranchName(state.branchName) === '' || state.isChecking
}
errorMessage={state.errorMessage}
onBranchNameChange={(branchName) => {
abortPreflight();
Expand Down
2 changes: 1 addition & 1 deletion packages/tinacms/src/toolkit/core/media-store.default.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import {
type MediaWorkflowConfirmBranchEvent,
getEditorialWorkflowPrTitle,
} from '@toolkit/form-builder/editorial-workflow-utils';
import { formatBranchName } from '@toolkit/plugin-branch-switcher/format-branch-name';
import type { TinaCMS } from '@toolkit/tina-cms';
import { formatBranchName } from '@utils/branch-name';
import type { Client } from '../../internalClient';
import {
E_BAD_ROUTE,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Form } from '@toolkit/forms';
import { formatBranchName } from '@toolkit/plugin-branch-switcher';
import { Button } from '@toolkit/styles';
import { formatBranchName, normalizeBranchName } from '@utils/branch-name';
import { CircleAlert, GitBranchIcon } from 'lucide-react';
import * as React from 'react';
import { useCMS } from '../react-core';
Expand Down Expand Up @@ -33,6 +33,7 @@ export const BranchDeletedModal = ({
const cms = useCMS();
const tinaApi = cms.api.tina;
const [newBranchName, setNewBranchName] = React.useState('');
const normalizedBranchName = normalizeBranchName(newBranchName);

const baseBranch =
tinaApi.protectedBranches[0] ||
Expand All @@ -50,7 +51,7 @@ export const BranchDeletedModal = ({

const handleCreate = async () => {
const { success } = await executeWorkflow({
branchName: `tina/${newBranchName}`,
branchName: `tina/${normalizedBranchName}`,
baseBranch,
path,
values,
Expand Down Expand Up @@ -130,7 +131,7 @@ export const BranchDeletedModal = ({
<Button
variant='primary'
className='w-full sm:w-auto'
disabled={!newBranchName}
disabled={!normalizedBranchName}
onClick={handleCreate}
>
<GitBranchIcon
Expand Down
37 changes: 7 additions & 30 deletions packages/tinacms/src/toolkit/form-builder/create-branch-modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ import { FieldLabel } from '@toolkit/fields';
import { Form } from '@toolkit/forms';
import { useLocalStorage } from '@toolkit/hooks/use-local-storage';
import { Button, DropdownButton } from '@toolkit/styles';
import {
formatDefaultBranchName,
normalizeBranchName,
} from '@utils/branch-name';
import {
CircleAlert,
Eye,
Expand Down Expand Up @@ -30,34 +34,6 @@ import {
} from './save-options';
import { useEditorialWorkflow } from './use-editorial-workflow';

// Format the default branch name by removing content/ prefix and file extension
const formatDefaultBranchName = (
filePath: string,
crudType: string
): string => {
let result = filePath;

const contentPrefix = 'content/';
// Remove "content/" prefix if present
if (result.startsWith(contentPrefix)) {
result = result.substring(contentPrefix.length);
}

// Remove file extension
const lastDot = result.lastIndexOf('.');
const lastSlash = Math.max(result.lastIndexOf('/'), result.lastIndexOf('\\'));
if (lastDot > lastSlash && lastDot > 0) {
result = result.slice(0, lastDot);
}

// Add deletion indicator for delete operations
if (crudType === 'delete') {
result = `❌-${result}`;
}

return result;
};

export const CreateBranchModal = ({
close,
safeSubmit,
Expand All @@ -82,6 +58,7 @@ export const CreateBranchModal = ({
);
const [isBranchGuardChecking, setIsBranchGuardChecking] =
React.useState(false);
const normalizedBranchName = normalizeBranchName(newBranchName);
const branchGuardAbortRef = React.useRef<AbortController | null>(null);

const {
Expand Down Expand Up @@ -112,7 +89,7 @@ export const CreateBranchModal = ({
setIsBranchGuardChecking(true);

const baseBranch = decodeURIComponent(tinaApi.branch);
const targetBranch = `tina/${newBranchName}`;
const targetBranch = `tina/${normalizedBranchName}`;

const { baseBranchExists, targetBranchExists } = await checkBranchGuard(
tinaApi,
Expand Down Expand Up @@ -183,7 +160,7 @@ export const CreateBranchModal = ({
close();
}}
errorMessage={errorMessage}
disabled={newBranchName === '' || isBranchGuardChecking}
disabled={normalizedBranchName === '' || isBranchGuardChecking}
onBranchNameChange={(value) => {
abortBranchGuard();
reset();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import { fireEvent, render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import * as React from 'react';
import { describe, expect, it, vi } from 'vitest';
import { BranchCreator } from './branch-switcher';
import { CreateBranch } from './branch-switcher-legacy';

const createButton = () =>
screen.getByRole('button', { name: /create branch/i }) as HTMLButtonElement;

// The toolkit Button renders `disabled` as styling, not the DOM attribute, so the
// button stays focusable. `click()` stands in for that keyboard activation path.
const isStyledDisabled = () =>
createButton().className.includes('cursor-not-allowed');

describe('BranchCreator (editorial workflow)', () => {
const renderCreator = () => {
const handleCreateBranch = vi.fn();
const { container } = render(
<BranchCreator
setViewState={vi.fn()}
handleCreateBranch={handleCreateBranch}
currentBranch='main'
/>
);
// the first textbox is the disabled "Current Branch Name" field
const input = screen.getAllByRole('textbox')[1];
return { handleCreateBranch, input, form: container.querySelector('form') };
};

it('disables create when the name normalises to empty', async () => {
const { input } = renderCreator();
await userEvent.type(input, '///');
expect(isStyledDisabled()).toBe(true);
});

it('does not create a branch when activated with a name that normalises to empty', async () => {
const { handleCreateBranch, input } = renderCreator();
await userEvent.type(input, '///');
createButton().click();
expect(handleCreateBranch).not.toHaveBeenCalled();
});

it('does not let form submission reload the page', () => {
const { form } = renderCreator();
const submitted = fireEvent.submit(form);
// fireEvent returns false when a handler called preventDefault
expect(submitted).toBe(false);
});

it('submits the normalised name under the tina/ prefix', async () => {
const { handleCreateBranch, input } = renderCreator();
await userEvent.type(input, '//My Branch//');
expect(isStyledDisabled()).toBe(false);
await userEvent.click(createButton());
expect(handleCreateBranch).toHaveBeenCalledWith('tina/my-branch');
});
});

describe('CreateBranch (legacy switcher)', () => {
const renderCreator = (newBranchName: string) => {
const onCreateBranch = vi.fn();
render(
<CreateBranch
currentBranch='main'
newBranchName={newBranchName}
onCreateBranch={onCreateBranch}
setNewBranchName={vi.fn()}
/>
);
return { onCreateBranch };
};

it('disables create when the name normalises to empty', () => {
renderCreator('///');
expect(isStyledDisabled()).toBe(true);
});

it('does not create a branch when activated with a name that normalises to empty', () => {
const { onCreateBranch } = renderCreator('///');
createButton().click();
expect(onCreateBranch).not.toHaveBeenCalled();
});

it('submits the normalised name', async () => {
const { onCreateBranch } = renderCreator('//My Branch//');
expect(isStyledDisabled()).toBe(false);
await userEvent.click(createButton());
expect(onCreateBranch).toHaveBeenCalledWith('my-branch');
});
});
Loading
Loading