From d8e9bd383029c142dff9ccea3f5a0c2e5954012e Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 7 Nov 2026 22:13:10 +0000 Subject: [PATCH 1/5] fix(sql): keep a name's line break from escaping a generated comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The script writes object and column names into `--` notes — the per-step header, review notes, manual-review blocks — and a comment ends at the first line break. A name carrying one closed its note and left the rest of itself on the next line, as a statement the migration then ran: -- CREATE VIEW: v DROP TABLE victim; -- CREATE OR REPLACE VIEW "v DROP TABLE victim; --" AS ... The splitter read `DROP TABLE victim;` as its own statement. A line break cannot be escaped inside a quoted identifier, so quoting does not help: to name that column the SQL has to carry the break literally. Reachable the same way as the other identifier injections — a source schema the user does not control — and dialect-independent. Fixed in three layers, because the names reach comments by three different routes: - `flattenIdentifier` in `ident`, where every name the generator quotes passes through. Covers the review notes and manual-review blocks that take a name from `ident` / `bareName` / `qualify` / `columnIdent`. - `MigrationStep.objectName`, now documented as single-line and flattened at all four assignment sites. The per-step header is emitted separately from the statements, so it is not covered by the above, and it was the route the first reproduction took. - `oneLineComment` on the notes that quote a name `ident` never saw — an index, constraint or trigger name, a type-translation warning — and `keepCommentCommented` as a backstop at the one place every step's statements pass through, for notes a dialect hook builds from values the generator never quotes. ClickHouse's ENGINE / ORDER BY note repeats the sorting key and the engine string; SQLite's column note repeats the type. All three were exploitable, and the backstop covers hooks added later too. The backstop only ever adds `-- ` to a line, and only inside a statement that already opens with `--`, so real DDL cannot be commented out. Verified as a no-op on valid output: across 1056 statements from every dialect, same- and cross-dialect, nothing opening with `--` carried a non-comment line. A comment and code never share a statement, because the step header is emitted separately. Swept rather than spot-checked: 99 shape x dialect combinations (each also cross-dialect) with the payload in every name field, 35 `generateObjectDdl` combinations, and newlines in a sorting key, engine string, column type and column default. All clean; before the fix, name fields, the two dialect notes and the cross-dialect column note were not. Trade-off: a name that really does contain a line break is emitted with spaces instead, so that one object needs a hand-written migration. Such a name cannot be escaped in SQL and is pathological; the alternative is running DDL that a source schema chose. npx vitest run 6925 passed, 1 expected fail, 145 skipped (was 6914) npx eslint . exit 0 npm run lint:security exit 0 npm run typecheck exit 0 10 of the 11 new tests fail on the unfixed generator; the eleventh asserts that ordinary output is unchanged and passes either way. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019ehcTb7vYMCawV1ajxwfxB --- .../migrations/sql-generator.module.test.ts | 102 +++++++++++++++++ .../migrations/sql-generator.module.ts | 104 +++++++++++++++--- 2 files changed, 189 insertions(+), 17 deletions(-) diff --git a/packages/sql/src/modules/migrations/sql-generator.module.test.ts b/packages/sql/src/modules/migrations/sql-generator.module.test.ts index dad1682a..54548d68 100644 --- a/packages/sql/src/modules/migrations/sql-generator.module.test.ts +++ b/packages/sql/src/modules/migrations/sql-generator.module.test.ts @@ -1870,3 +1870,105 @@ describe('ClickHouse ENGINE clause: identifiers are quoted, catalog expressions }); }); }); + +describe('a line break in a name cannot escape a generated comment', () => { + // The script writes names into `--` notes — the per-step header, review notes, + // manual-review blocks — and a comment ends at the first line break, so a + // name carrying one left the rest of itself on the next line as a statement + // the migration then ran. + const PAYLOAD = '\nDROP TABLE victim; --'; + const t = (over: Partial & { name: string }) => tableSchema(over); + const c = (name: string, type = 'INTEGER') => ({ name, type, nullable: true, primaryKey: false }); + /** Statements that would actually execute the payload. */ + const escaped = (sql: string) => + splitSqlStatements(sql).filter((s) => /^\s*DROP\s+TABLE\s+victim/i.test(s.text)); + + it('keeps the per-step header on one line', () => { + const diff: TableDiff = { + tableName: `v${PAYLOAD}`, + objectType: 'VIEW', + status: 'ADDED', + definition: 'SELECT 1', + columnDiffs: [], + indexDiffs: [], + foreignKeyDiffs: [], + }; + const sql = gen.generateMigrationSql([diff], 'postgres'); + expect(escaped(sql)).toHaveLength(0); + // The name still reaches the DDL, flattened, so the object is identifiable. + expect(sql).toContain('"v DROP TABLE victim; --"'); + }); + + it.each(['postgres', 'sqlite', 'mysql', 'db2', 'sqlserver', 'clickhouse'])( + 'quotes a column name carrying one on %s', + (dialect) => { + const ddl = gen.generateObjectDdl( + t({ name: 'orders', columns: [c(`col${PAYLOAD}`), c('ok_col')] }), + dialect + ); + expect(escaped(ddl)).toHaveLength(0); + expect(ddl).not.toMatch(/^\s*DROP\s+TABLE/im); + } + ); + + it('survives a review note built by a dialect from a column type', () => { + // SQLite cannot ALTER a column type, so it emits `-- review: … `. + const diff: TableDiff = { + tableName: 'x', + objectType: 'TABLE', + status: 'MODIFIED', + indexDiffs: [], + foreignKeyDiffs: [], + columnDiffs: [ + { + name: 'col', + status: 'MODIFIED', + source: { name: 'col', type: `TEXT${PAYLOAD}`, nullable: false, primaryKey: false }, + target: { name: 'col', type: 'INTEGER', nullable: true, primaryKey: false }, + }, + ], + }; + expect(escaped(gen.generateMigrationSql([diff], 'sqlite'))).toHaveLength(0); + }); + + it('survives a review note built by a dialect from a sorting key', () => { + // ClickHouse cannot ALTER ENGINE / ORDER BY, so it repeats them in a note. + const table = (over: Partial) => + t({ name: 'x', engine: 'MergeTree', columns: [c('a', 'Int32')], ...over }); + const diff: TableDiff = { + tableName: 'x', + objectType: 'TABLE', + status: 'MODIFIED', + columnDiffs: [], + indexDiffs: [], + foreignKeyDiffs: [], + sourceTable: table({ orderBy: [`a${PAYLOAD}`] }), + targetTable: table({ engine: 'ReplacingMergeTree', orderBy: ['b'] }), + }; + expect(escaped(gen.generateMigrationSql([diff], 'clickhouse'))).toHaveLength(0); + }); + + it('survives a cross-dialect manual-review block', () => { + const diff: TableDiff = { + tableName: `v${PAYLOAD}`, + objectType: 'VIEW', + status: 'ADDED', + definition: 'SELECT 1', + columnDiffs: [], + indexDiffs: [], + foreignKeyDiffs: [], + }; + const sql = gen.generateMigrationSql([diff], 'postgres', { sourceDialect: 'oracle' }); + expect(escaped(sql)).toHaveLength(0); + }); + + it('leaves ordinary output untouched', () => { + const ddl = gen.generateObjectDdl( + t({ name: 'orders', primaryKey: { name: 'PK', columns: ['id'] }, columns: [c('id'), c('name', 'TEXT')] }), + 'postgres' + ); + expect(ddl).toContain('CREATE TABLE orders ('); + expect(ddl).toContain('id INTEGER'); + expect(ddl).not.toMatch(/-- {2}/); + }); +}); diff --git a/packages/sql/src/modules/migrations/sql-generator.module.ts b/packages/sql/src/modules/migrations/sql-generator.module.ts index e0772737..ddcb2437 100644 --- a/packages/sql/src/modules/migrations/sql-generator.module.ts +++ b/packages/sql/src/modules/migrations/sql-generator.module.ts @@ -12,6 +12,12 @@ const selectionKey = (d: Pick): string => `${d.objectType}\u0000${d.tableName}`; export interface MigrationStep { + /** + * Always a single line: the script heads each step with + * `-- : `, and a line break in a name would end + * that comment and leave the rest of the name executable. See + * `flattenIdentifier`. + */ objectName: string; objectType: DbObjectType; action: 'DROP' | 'CREATE' | 'ALTER'; @@ -123,6 +129,66 @@ function ansiQuoteIdentifier(name: string): string { * follows is not part of the name. That name is not well-formed, and `ident` * sends it to the dialect's quoter to be escaped from scratch instead. */ +/** + * An identifier with its line breaks turned into spaces. + * + * A line break cannot be escaped inside a quoted identifier — to name that + * column the generated SQL has to carry the break literally — and the script + * writes names into `--` comments, which end at the first one. A table named + * + * x⏎DROP TABLE victim; -- + * + * therefore closed its `-- review:` note and left the rest of itself on the + * next line as a statement of its own, which the migration then ran. Several + * notes carry a name (review notes, manual-review blocks, the per-step + * header), so this is fixed here, at the one point every name passes through, + * rather than at each of them. + * + * The trade-off is deliberate: a name that really does contain a line break is + * emitted with spaces instead, so that object needs a hand-written migration. + * Such a name is pathological — and the alternative is executing DDL that a + * source schema chose. + */ +function flattenIdentifier(name: string): string { + return /[\r\n]/.test(name) ? name.replace(/[\r\n]+/g, ' ') : name; +} + +/** + * A whole `--` note on one line, for the review notes that quote a name the + * generator did not put through `ident` — an index, constraint or trigger name, + * or a type-translation warning that repeats one. Same reason as + * `flattenIdentifier`: the note ends at the first line break, and anything + * after it is a statement. + */ +function oneLineComment(text: string): string { + return /[\r\n]/.test(text) ? text.replace(/[\r\n]+/g, ' ') : text; +} + +/** + * A statement that opens with `--` stays commented to its last line. + * + * The backstop behind `flattenIdentifier` and `oneLineComment`. Those cover the + * names and notes this module writes, but a dialect hook builds its own review + * notes out of values the generator never quotes — ClickHouse's ENGINE / ORDER BY + * note repeats the sorting key, SQLite's column note repeats the type — and a + * line break in any of them ends the note and leaves the rest executable. + * Catching it once here covers every hook, including ones added later. + * + * Safe because a comment and code never share a statement: the per-step + * `-- ACTION TYPE: name` header is emitted separately, not pushed as part of + * one. Checked over 1056 statements across every dialect, same- and + * cross-dialect, where nothing opening with `--` carried a non-comment line, so + * this is a no-op on valid output. A statement that does not open with `--` is + * never touched, so real DDL cannot be commented out. + */ +function keepCommentCommented(statement: string): string { + const lines = statement.split(/\r\n|\r|\n/); + const isComment = (line: string) => !line.trim() || line.trimStart().startsWith('--'); + if (lines.length < 2 || !lines[0]!.trimStart().startsWith('--')) return statement; + if (lines.every(isComment)) return statement; + return lines.map((line, i) => (i === 0 || isComment(line) ? line : `-- ${line}`)).join('\n'); +} + function isWellFormedQuotedIdentifier(name: string): boolean { if (name.length < 2) return false; const open = name[0]!; @@ -170,8 +236,9 @@ export class SqlGeneratorModule { * without rewriting the dialect test suites. */ private ident(name: string): string { - if (isBareIdentifier(name) || isWellFormedQuotedIdentifier(name)) return name; - return this.quoteIdentifier(name); + const flat = flattenIdentifier(name); + if (isBareIdentifier(flat) || isWellFormedQuotedIdentifier(flat)) return flat; + return this.quoteIdentifier(flat); } /** @@ -400,7 +467,7 @@ export class SqlGeneratorModule { // rather than shipping DDL that cannot run. const hidden = idx.columns.find((c) => /^SYS_NC\d+\$$/i.test((c ?? '').trim())); if (hidden) { - return `-- review: skip index ${idx.name} on ${qualifiedTable} — function-based index; its expression is not captured (column reads as ${hidden})`; + return oneLineComment(`-- review: skip index ${idx.name} on ${qualifiedTable} — function-based index; its expression is not captured (column reads as ${hidden})`); } // Quote the column names *before* handing them to a dialect hook: the hooks // build their own column list, so a hook-owning dialect (SQLite, SQL Server) @@ -428,7 +495,7 @@ export class SqlGeneratorModule { const refCols = referencedColumns ?? []; const label = constraintName || '(unnamed)'; if (columns.length === 0 || refCols.length !== columns.length) { - return `-- review: skip FK ${label} — referenced columns missing or length mismatch`; + return oneLineComment(`-- review: skip FK ${label} — referenced columns missing or length mismatch`); } // SQLite and ClickHouse have no ALTER TABLE ... ADD CONSTRAINT. The matrix // already knew that and only the blueprint UI was reading it, so the @@ -436,7 +503,7 @@ export class SqlGeneratorModule { // DDL that cannot run. const fk = this.fkSupport(); if (!fk.alterAdd) { - return `-- review: skip FK ${label} — ${fk.hint}`; + return oneLineComment(`-- review: skip FK ${label} — ${fk.hint}`); } const cols = columns.map((c) => this.ident(c)).join(', '); const parentCols = refCols.map((c) => this.ident(c)).join(', '); @@ -487,7 +554,7 @@ export class SqlGeneratorModule { if (table.objectType === 'SEQUENCE') return this.renderCreateSequence(table, dialect); if (table.objectType === 'TYPE') return this.renderCreateType(table, dialect); if (table.objectType !== 'TABLE' && table.objectType !== 'MQT') { - return table.definition || `-- No definition available for ${table.objectType} ${table.name}`; + return table.definition || oneLineComment(`-- No definition available for ${table.objectType} ${table.name}`); } let sql = this.renderCreateTable(table, dialect) + `\n`; @@ -513,7 +580,7 @@ export class SqlGeneratorModule { // unterminated CREATE TRIGGER swallows whatever statement follows it. sql += trg.definition ? `${this.ensureSemicolon(trg.definition)}\n` - : `-- Trigger ${trg.name} (${trg.timing ?? ''} ${trg.event ?? ''}) has no available definition\n`; + : `-- Trigger ${flattenIdentifier(trg.name)} (${trg.timing ?? ''} ${trg.event ?? ''}) has no available definition\n`; } return sql; @@ -568,7 +635,7 @@ export class SqlGeneratorModule { if (this.fkCycleKeys.has(cycleKey)) { const cycleList = [...this.fkCycleKeys].sort().join(', '); statements.push( - `-- review: FK cycle involving ${cycleList} — create order may fail; break the cycle manually` + oneLineComment(`-- review: FK cycle involving ${cycleList} — create order may fail; break the cycle manually`) ); } // Postgres matviews (definition captured + dialect hook available) render as a @@ -579,7 +646,7 @@ export class SqlGeneratorModule { obj.objectType === 'MQT' && source.definition && dialect.createMaterializedViewStatement ? dialect.createMaterializedViewStatement(name, this.ensureSemicolon(source.definition)) : this.renderCreateTable(source, dialect, mapping, typeWarnings); - for (const w of typeWarnings) statements.push(`-- review: ${w}`); + for (const w of typeWarnings) statements.push(oneLineComment(`-- review: ${w}`)); // Postgres serial columns reference a backing sequence in their DEFAULT // (nextval('…_seq'::regclass)). That sequence must exist before the table @@ -812,7 +879,7 @@ export class SqlGeneratorModule { for (const col of obj.columnDiffs.filter((c) => c.status === 'ADDED')) { if (!col.source) continue; const translated = this.translateType(col.source.type, mapping); - if (translated.warning) statements.push(`-- review: ${col.name}: ${translated.warning}`); + if (translated.warning) statements.push(oneLineComment(`-- review: ${col.name}: ${translated.warning}`)); // A serial column added to an existing table needs its sequence first. const seq = dialect.serialSequenceFromDefault?.(col.source.defaultValue ?? ''); if (seq) statements.push(`CREATE SEQUENCE IF NOT EXISTS ${this.qualify(seq, mapping)};`); @@ -848,7 +915,7 @@ export class SqlGeneratorModule { if (!col.source) continue; if (col.target && col.source.type !== col.target.type) retypedColumns = true; const translated = this.translateType(col.source.type, mapping); - if (translated.warning) statements.push(`-- review: ${col.name}: ${translated.warning}`); + if (translated.warning) statements.push(oneLineComment(`-- review: ${col.name}: ${translated.warning}`)); // Cross-dialect: the raw default AND collation are source-dialect vocabulary // (collation names don't map across engines any more than default expressions // do) — strip both so a hook never emits either verbatim into the target @@ -869,7 +936,7 @@ export class SqlGeneratorModule { if (this.isCrossDialect(mapping) && srcDef) { // A default expression is source-dialect SQL; don't risk emitting it // verbatim into another dialect — flag it for review instead. - statements.push(`-- review: ${col.name}: default '${srcDef}' is ${(mapping?.sourceDialect ?? '').toUpperCase()} syntax — set it manually for ${(mapping?.targetDialect ?? '').toUpperCase()}`); + statements.push(oneLineComment(`-- review: ${col.name}: default '${srcDef}' is ${(mapping?.sourceDialect ?? '').toUpperCase()} syntax — set it manually for ${(mapping?.targetDialect ?? '').toUpperCase()}`)); } else { // A new serial/sequence default needs its backing sequence to exist first. const seqForDefault = dialect.serialSequenceFromDefault?.(srcDef ?? ''); @@ -1204,7 +1271,7 @@ export class SqlGeneratorModule { !obj.definition && !this.isCrossDialect(m); return { - objectName: obj.tableName, + objectName: flattenIdentifier(obj.tableName), objectType: obj.objectType, action: 'CREATE', statements: stmts, @@ -1260,7 +1327,7 @@ export class SqlGeneratorModule { // Non-destructive mode never drops objects that exist only in the target. if (!m.nonDestructive) { for (const obj of this.sortRemovedByDependency(diffs.filter((d) => d.status === 'REMOVED'))) { - steps.push({ objectName: obj.tableName, objectType: obj.objectType, action: 'DROP', statements: this.dropObjectStatements(obj, m, dialect) }); + steps.push({ objectName: flattenIdentifier(obj.tableName), objectType: obj.objectType, action: 'DROP', statements: this.dropObjectStatements(obj, m, dialect) }); } } @@ -1276,7 +1343,7 @@ export class SqlGeneratorModule { const tables = addedStructural.filter((d) => d.objectType === 'TABLE' || d.objectType === 'MQT'); for (const obj of [...supporting, ...this.sortAddedByDependency(tables)]) { const stmts = this.createObjectStatements(obj, dialect, m); - steps.push({ objectName: obj.tableName, objectType: obj.objectType, action: 'CREATE', statements: stmts }); + steps.push({ objectName: flattenIdentifier(obj.tableName), objectType: obj.objectType, action: 'CREATE', statements: stmts }); } // ADDED routines (FUNCTION, PROCEDURE) come before MODIFIED — a MODIFIED table's @@ -1292,7 +1359,7 @@ export class SqlGeneratorModule { const selectedKeys = new Set(diffs.map(selectionKey)); for (const obj of diffs.filter((d) => d.status === 'MODIFIED')) { steps.push({ - objectName: obj.tableName, + objectName: flattenIdentifier(obj.tableName), objectType: obj.objectType, action: 'ALTER', statements: this.alterObjectStatements(obj, dialect, m, depContext, selectedKeys), @@ -1316,7 +1383,10 @@ export class SqlGeneratorModule { const actionable = steps .filter((s) => s.statements.length > 0) - .map((s) => ({ ...s, statements: s.statements.map((st) => this.remapSchema(st, m)) })); + .map((s) => ({ + ...s, + statements: s.statements.map((st) => keepCommentCommented(this.remapSchema(st, m))), + })); // Attach skipped list so generateMigrationSql can report it. (actionable as any).__skipped = skippedInNonDestructive; From 95f416dd56d838b3b0fc38e442405214b29d577b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 7 Nov 2026 22:25:27 +0000 Subject: [PATCH 2/5] chore(web): hold Workflow back from the rail for 0.3.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Workflow is a large feature and is not ready to be offered, so it comes off the activity rail and out of the command palette for the release. `NAV_FLAGS.workflow` in the web feature registry already existed for exactly this, so the change is that one flag; turning it back to `true` is the whole of putting the feature back. The view and `/api/workflow` are untouched, as are the engine and apps/workflow-server. Note what that does and does not mean: a view is reached only through the rail or the command palette — there is no URL for one — so with the flag off the Workflow view cannot be opened from the UI at all. The API stays mounted and still answers an API client, and the Home Services card still reports engine health. Tests follow the flag rather than being loosened: - The rail and command-palette assertions no longer list Workflow. The ActivityRail spec now asserts the button is absent, and its click-through check moved to the Access button so "a rail button switches view" is still covered. - The two Workflow e2e specs drive the view through the rail, which no longer exists, so they are `describe.skipIf(!WORKFLOW_ON_RAIL)`. That constant mirrors the flag, so flipping the flag re-enables them unchanged rather than leaving a silent skip behind. npx vitest run 6925 passed, 1 expected fail, 145 skipped npx eslint . exit 0 npm run typecheck exit 0 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019ehcTb7vYMCawV1ajxwfxB --- apps/e2e/src/helpers/features.ts | 19 +++++++++++++++++++ .../tests/workflow-designer-roundtrip.test.ts | 4 +++- apps/e2e/src/tests/workflow-smoke.test.ts | 4 +++- .../app/features/featureRegistry.test.ts | 4 ++-- .../frontend/app/features/featureRegistry.ts | 12 ++++++++++-- .../frontend/app/shell/ActivityRail.test.tsx | 7 ++++--- .../app/shell/CommandPalette.test.tsx | 3 ++- 7 files changed, 43 insertions(+), 10 deletions(-) create mode 100644 apps/e2e/src/helpers/features.ts diff --git a/apps/e2e/src/helpers/features.ts b/apps/e2e/src/helpers/features.ts new file mode 100644 index 00000000..c094a5f1 --- /dev/null +++ b/apps/e2e/src/helpers/features.ts @@ -0,0 +1,19 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + * + * Features the UI currently offers, for specs that would otherwise fail on a + * feature the app deliberately hides. + */ + +/** + * Whether Workflow is on the activity rail — mirrors `NAV_FLAGS.workflow` in + * `apps/web/src/frontend/app/features/featureRegistry.ts`, held back for 0.3.1. + * + * A view is reached only through the rail or the command palette; there is no + * URL for one. With the flag off the Workflow view cannot be opened at all, so + * its specs have nothing to drive. Turn this back to `true` with the flag and + * they run again unchanged. + */ +export const WORKFLOW_ON_RAIL = false; diff --git a/apps/e2e/src/tests/workflow-designer-roundtrip.test.ts b/apps/e2e/src/tests/workflow-designer-roundtrip.test.ts index 229b12a6..293bb0fe 100644 --- a/apps/e2e/src/tests/workflow-designer-roundtrip.test.ts +++ b/apps/e2e/src/tests/workflow-designer-roundtrip.test.ts @@ -18,6 +18,7 @@ import { buildDriver, clickWhen, quitDriver, waitFor } from '../helpers/driver.j import { byTestId } from '../helpers/test-ids.js'; import { AppPage } from '../pages/AppPage.js'; import { WorkflowPage } from '../pages/WorkflowPage.js'; +import { WORKFLOW_ON_RAIL } from '../helpers/features.js'; const ENGINE = 'http://127.0.0.1:8081/api'; const ID = 'e2e-designer-carry'; @@ -80,7 +81,8 @@ afterAll(async () => { } }); -describe('workflow designer round trip', () => { +// Workflow is off the rail for 0.3.1, so the view cannot be opened; see WORKFLOW_ON_RAIL. +describe.skipIf(!WORKFLOW_ON_RAIL)('workflow designer round trip', () => { it('keeps every field it does not edit through a load and a save', async (ctx) => { if (!engineUp) ctx.skip(); await new AppPage(driver).open(); diff --git a/apps/e2e/src/tests/workflow-smoke.test.ts b/apps/e2e/src/tests/workflow-smoke.test.ts index 7d72a45b..3e2c6f2c 100644 --- a/apps/e2e/src/tests/workflow-smoke.test.ts +++ b/apps/e2e/src/tests/workflow-smoke.test.ts @@ -8,6 +8,7 @@ import type { Page } from 'playwright'; import { buildDriver, quitDriver, BASE_URL } from '../helpers/driver.js'; import { AppPage } from '../pages/AppPage.js'; import { WorkflowPage } from '../pages/WorkflowPage.js'; +import { WORKFLOW_ON_RAIL } from '../helpers/features.js'; let driver: Page; @@ -19,7 +20,8 @@ afterAll(async () => { await quitDriver(driver); }); -describe('Workflow workspace', () => { +// Workflow is off the rail for 0.3.1, so the view cannot be opened; see WORKFLOW_ON_RAIL. +describe.skipIf(!WORKFLOW_ON_RAIL)('Workflow workspace', () => { it('opens from the activity rail and shows owner panes', async () => { const app = new AppPage(driver); await app.open(); diff --git a/apps/web/src/frontend/app/features/featureRegistry.test.ts b/apps/web/src/frontend/app/features/featureRegistry.test.ts index ab2ff669..90314e18 100644 --- a/apps/web/src/frontend/app/features/featureRegistry.test.ts +++ b/apps/web/src/frontend/app/features/featureRegistry.test.ts @@ -76,7 +76,7 @@ describe('the rail', () => { ['sqlEditor', 'Editor', 'view-sql-editor-btn'], ['utilities', 'Utils', 'view-utilities-btn'], ['access', 'Access', 'view-access-btn'], - ['workflow', 'Workflow', 'view-workflow-btn'], + // Workflow is held back for 0.3.1 (NAV_FLAGS.workflow === false). ['snapshots', 'Snapshots', 'sync-pane-history-btn'], ]); }); @@ -86,7 +86,7 @@ describe('the rail', () => { 'sync', 'sqlEditor', 'access', - 'workflow', + // no 'workflow': held back for 0.3.1 (NAV_FLAGS.workflow === false). 'snapshots', ]); }); diff --git a/apps/web/src/frontend/app/features/featureRegistry.ts b/apps/web/src/frontend/app/features/featureRegistry.ts index 61a90a30..5517414a 100644 --- a/apps/web/src/frontend/app/features/featureRegistry.ts +++ b/apps/web/src/frontend/app/features/featureRegistry.ts @@ -114,8 +114,16 @@ export const WORKSPACE_VIEWS: Record = { settings: { load: () => import('@/app/settings/SettingsView') }, }; -/** Workflow is on in this edition; flags gate nav items, not routes. */ -const NAV_FLAGS = { workflow: true } as const; +/** + * Flags gate nav items, not routes. + * + * Workflow is held back for 0.3.1: it is a large feature and is not ready to + * be offered yet, so it is off the rail and out of the command palette. The + * view and `/api/workflow` are untouched — a direct URL still reaches it, and + * the engine still answers — so turning this back to `true` is the whole of + * putting it back. + */ +const NAV_FLAGS = { workflow: false } as const; export interface RailItem { view: ActiveView; diff --git a/apps/web/src/frontend/app/shell/ActivityRail.test.tsx b/apps/web/src/frontend/app/shell/ActivityRail.test.tsx index 23af64b3..d9f8d868 100644 --- a/apps/web/src/frontend/app/shell/ActivityRail.test.tsx +++ b/apps/web/src/frontend/app/shell/ActivityRail.test.tsx @@ -40,14 +40,15 @@ describe('ActivityRail', () => { expect(screen.getByTestId('view-sql-editor-btn')).toBeTruthy(); expect(screen.getByTestId('view-utilities-btn')).toBeTruthy(); expect(screen.getByTestId('view-access-btn')).toBeTruthy(); - expect(screen.getByTestId('view-workflow-btn')).toBeTruthy(); + // Workflow is held back for 0.3.1 (NAV_FLAGS.workflow === false). + expect(screen.queryByTestId('view-workflow-btn')).toBeNull(); expect(screen.getByTestId('sync-pane-history-btn')).toBeTruthy(); expect(screen.getByTestId('credentials-btn')).toBeTruthy(); expect(screen.getByTestId('history-btn')).toBeTruthy(); expect(screen.getByTestId('workspace-rail-btn')).toBeTruthy(); expect(screen.getByTestId('profile-menu-trigger')).toBeTruthy(); - fireEvent.click(screen.getByTestId('view-workflow-btn')); - expect(useUiStore.getState().activeView).toBe('workflow'); + fireEvent.click(screen.getByTestId('view-access-btn')); + expect(useUiStore.getState().activeView).toBe('access'); fireEvent.click(screen.getByTestId('home-open-btn')); expect(useUiStore.getState().activeView).toBe('home'); fireEvent.click(screen.getByTestId('view-settings-btn')); diff --git a/apps/web/src/frontend/app/shell/CommandPalette.test.tsx b/apps/web/src/frontend/app/shell/CommandPalette.test.tsx index f2a3e6a5..4e3d42e5 100644 --- a/apps/web/src/frontend/app/shell/CommandPalette.test.tsx +++ b/apps/web/src/frontend/app/shell/CommandPalette.test.tsx @@ -95,7 +95,8 @@ describe('CommandPalette', () => { const ids = screen .getAllByTestId(/^command-palette-item-ws-/) .map((el) => el.getAttribute('data-testid')!.replace('command-palette-item-', '')); - expect(ids).toEqual(['ws-home', 'ws-sync', 'ws-sqlEditor', 'ws-utilities', 'ws-access', 'ws-workflow', 'ws-snapshots', 'ws-settings']); + // No 'ws-workflow': held back for 0.3.1 (NAV_FLAGS.workflow === false). + expect(ids).toEqual(['ws-home', 'ws-sync', 'ws-sqlEditor', 'ws-utilities', 'ws-access', 'ws-snapshots', 'ws-settings']); expect(screen.getByTestId('command-palette-item-ws-sqlEditor').textContent).toContain('SQL Editor'); expect(screen.getByTestId('command-palette-item-ws-utilities').textContent).toContain('Utilities'); }); From f4f9f488db31cafe220b50d24312151eadc0cef7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 8 Nov 2026 06:40:01 +0000 Subject: [PATCH 3/5] feat(utilities): show which lines name a column being renamed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The column change screen listed the objects a rename touches — "View v_open_orders may use it", "Procedure bump_order_status may use it" — and left it there. The engine renames the column; it does not rewrite the SQL inside a view or a routine that reads it, so somebody then had to open each object and hunt for the name themselves. The impact now carries the lines. `columnReferenceLines` returns, for each dependent object, the 1-based line number, the line as written, where on the line the name sits, and — when the change renames — the line as it will have to read. A new read-only panel shows them, with the whole definition behind a toggle. On the demo schema, renaming customers.tier: VIEW v_customer_orders — 2 line(s) 1 | SELECT c.id AS customer_id, c.name, c.email, c.tier, → SELECT c.id AS customer_id, c.name, c.email, c.customer_tier, 6 | GROUP BY c.id, c.name, c.email, c.tier → GROUP BY c.id, c.name, c.email, c.customer_tier Read-only on purpose: Fox does not edit somebody else's routine, and the panel says so where a rename is planned. Covers views, functions, procedures and triggers. Notes on the implementation: - One filter feeds both the existing name lists and `dependents`, so the two cannot drift. The name lists are untouched: the warnings and the summary read them. - Matching keeps the semantics the impact already had — whole word, case insensitive, plain text — so a name inside a string literal or a comment counts. That errs towards showing a line somebody can dismiss over hiding one they needed, and the panel calls these references, not edits. - The matcher uses lookarounds rather than the consuming boundary groups `wordIn` uses: a consuming group swallows the character between two adjacent occurrences and reports one of them, and these offsets are what the UI highlights. `SET status = status` marks both. - The UI highlights from the offsets the plan reported rather than searching again, so the highlight cannot disagree with what the plan found. - Types reach the frontend through @foxschema/ui-shared, not @foxschema/sql directly — architecture.test.ts enforces that, and caught it. - `npm run test-ids` regenerated for the new data-testids. npx vitest run 6942 passed, 1 expected fail, 145 skipped (was 6925) npx eslint . exit 0 npm run lint:security exit 0 npm run typecheck exit 0 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019ehcTb7vYMCawV1ajxwfxB --- apps/e2e/src/generated/test-ids.ts | 20 +++ .../components/ColumnChangePanel.tsx | 3 + .../components/ColumnReferences.test.tsx | 114 +++++++++++++++ .../utilities/components/ColumnReferences.tsx | 135 ++++++++++++++++++ .../src/frontend/shared/lib/column-change.ts | 2 + docs/testing/TEST_IDS.md | 9 +- packages/sql/src/index.ts | 3 + .../modules/utilities/column-change.test.ts | 93 +++++++++++- .../src/modules/utilities/column-change.ts | 96 +++++++++++-- .../modules/utilities/column-change.types.ts | 35 +++++ packages/ui-shared/src/index.ts | 2 + 11 files changed, 496 insertions(+), 16 deletions(-) create mode 100644 apps/web/src/frontend/features/utilities/components/ColumnReferences.test.tsx create mode 100644 apps/web/src/frontend/features/utilities/components/ColumnReferences.tsx diff --git a/apps/e2e/src/generated/test-ids.ts b/apps/e2e/src/generated/test-ids.ts index a96179e5..7924d270 100644 --- a/apps/e2e/src/generated/test-ids.ts +++ b/apps/e2e/src/generated/test-ids.ts @@ -3038,6 +3038,20 @@ export const TestIds = { /** ul */ columnChangeWarnings: 'column-change-warnings', }, + ColumnReferences: { + /** section · Where is named — in */ + columnChangeReferences: 'column-change-references', + /** article */ + columnRefs: (toLowerCase: string | number, name: string | number) => `column-refs-${toLowerCase}-${name}`, + /** span */ + columnRefsAfter: (toLowerCase: string | number, name: string | number, line: string | number) => `column-refs-${toLowerCase}-${name}-after-${line}`, + /** pre */ + columnRefsDefinition: (toLowerCase: string | number, name: string | number) => `column-refs-${toLowerCase}-${name}-definition`, + /** ul */ + columnRefsLines: (toLowerCase: string | number, name: string | number) => `column-refs-${toLowerCase}-${name}-lines`, + /** button */ + columnRefsToggle: (toLowerCase: string | number, name: string | number) => `column-refs-${toLowerCase}-${name}-toggle`, + }, DatabaseAccessModal: { /** div */ dbAccessAllForm: 'db-access-all-form', @@ -4177,6 +4191,7 @@ export type TestId = | `column-change-part-${string}-copy` | 'column-change-password' | 'column-change-progress' + | 'column-change-references' | 'column-change-refused' | `column-change-run-${string}` | 'column-change-run-all' @@ -4188,6 +4203,11 @@ export type TestId = | 'column-change-table' | 'column-change-unlock' | 'column-change-warnings' + | `column-refs-${string}-${string}` + | `column-refs-${string}-${string}-after-${string}` + | `column-refs-${string}-${string}-definition` + | `column-refs-${string}-${string}-lines` + | `column-refs-${string}-${string}-toggle` | 'command-palette' | 'command-palette-btn' | 'command-palette-input' diff --git a/apps/web/src/frontend/features/utilities/components/ColumnChangePanel.tsx b/apps/web/src/frontend/features/utilities/components/ColumnChangePanel.tsx index c8297732..623351cd 100644 --- a/apps/web/src/frontend/features/utilities/components/ColumnChangePanel.tsx +++ b/apps/web/src/frontend/features/utilities/components/ColumnChangePanel.tsx @@ -27,6 +27,7 @@ import { import { splitSqlStatements } from '@/shared/lib/sql-splitter'; import { needsSessionPassword } from '@/shared/lib/provider-settings'; import { sectionLabelCls } from '@/shared/components/surfaces'; +import { ColumnReferences } from './ColumnReferences'; import type { TableSchema } from '@/shared/lib/types'; interface Props { @@ -502,6 +503,8 @@ export const ColumnChangePanel: React.FC = ({ lockedConnectionId = '' }) + + {conversionWarning && (
'done'", + references: [ + { + line: 2, + text: 'SELECT id, status FROM orders', + matches: [{ start: 11, end: 17 }], + renamed: 'SELECT id, order_status FROM orders', + }, + { + line: 3, + text: "WHERE status <> 'done'", + matches: [{ start: 6, end: 12 }], + renamed: "WHERE order_status <> 'done'", + }, + ], +}; + +const proc: ColumnDependent = { + name: 'bump_order_status', + objectType: 'PROCEDURE', + definition: 'UPDATE orders SET status = status', + references: [ + { + line: 1, + text: 'UPDATE orders SET status = status', + matches: [ + { start: 18, end: 24 }, + { start: 27, end: 33 }, + ], + }, + ], +}; + +describe('ColumnReferences', () => { + it('renders nothing when no object depends on the column', () => { + const { container } = render(); + expect(container.firstChild).toBeNull(); + }); + + it('counts the lines and objects it found', () => { + render(); + const heading = screen.getByTestId('column-change-references').textContent ?? ''; + expect(heading).toContain('3 lines'); + expect(heading).toContain('2 objects'); + }); + + it('shows each referencing line with its line number', () => { + render(); + const lines = screen.getByTestId('column-refs-view-v_open_orders-lines').textContent ?? ''; + expect(lines).toContain('2'); + expect(lines).toContain('SELECT id, status FROM orders'); + expect(lines).toContain('3'); + expect(lines).toContain("WHERE status <> 'done'"); + }); + + it('highlights every occurrence on a line, from the plan’s offsets', () => { + render(); + const marks = screen + .getByTestId('column-refs-procedure-bump_order_status') + .querySelectorAll('mark'); + // `SET status = status` — both, not just the first. + expect([...marks].map((m) => m.textContent)).toEqual(['status', 'status']); + }); + + it('shows the line as it will have to read when renaming', () => { + render(); + expect(screen.getByTestId('column-refs-view-v_open_orders-after-2').textContent).toBe( + 'SELECT id, order_status FROM orders' + ); + // And says plainly that Fox will not do it for you. + expect(screen.getByTestId('column-refs-view-v_open_orders').textContent).toContain( + 'Fox does not rewrite this view' + ); + }); + + it('offers no after-line and no rewrite note for a type change', () => { + render(); + const el = screen.getByTestId('column-refs-procedure-bump_order_status'); + expect(el.querySelector('[data-testid$="-after-1"]')).toBeNull(); + expect(el.textContent).not.toContain('does not rewrite'); + }); + + it('keeps the whole definition behind a toggle', () => { + render(); + expect(screen.queryByTestId('column-refs-view-v_open_orders-definition')).toBeNull(); + fireEvent.click(screen.getByTestId('column-refs-view-v_open_orders-toggle')); + expect(screen.getByTestId('column-refs-view-v_open_orders-definition').textContent).toBe(view.definition); + }); + + it('labels each kind of object', () => { + render( + + ); + const text = screen.getByTestId('column-change-references').textContent ?? ''; + for (const label of ['View', 'Procedure', 'Function', 'Trigger']) expect(text).toContain(label); + }); +}); diff --git a/apps/web/src/frontend/features/utilities/components/ColumnReferences.tsx b/apps/web/src/frontend/features/utilities/components/ColumnReferences.tsx new file mode 100644 index 00000000..19908b0e --- /dev/null +++ b/apps/web/src/frontend/features/utilities/components/ColumnReferences.tsx @@ -0,0 +1,135 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + * + * Where a column is named inside the views, routines and triggers that read it. + * + * The engine renames a column; it does not rewrite the SQL inside a view or a + * routine. "View v_open_orders may use it" tells somebody there is work without + * telling them where, so this shows the lines: as written, and — when the change + * renames — as they will have to read. Read-only on purpose; Fox does not edit + * somebody else's routine, it shows them what to change. + */ +import React, { useState } from 'react'; +import { ChevronDown, ChevronRight } from 'lucide-react'; +import type { ColumnDependent, ColumnReferenceLine } from '@/shared/lib/column-change'; +import { sectionLabelCls } from '@/shared/components/surfaces'; + +const KIND_LABEL: Record = { + VIEW: 'View', + FUNCTION: 'Function', + PROCEDURE: 'Procedure', + TRIGGER: 'Trigger', +}; + +/** + * A line with its matches marked, split on the offsets the plan reported rather + * than searched for again here: one matcher, in `columnReferenceLines`, so the + * highlight cannot disagree with what the plan found. + */ +const Highlighted: React.FC<{ line: ColumnReferenceLine }> = ({ line }) => { + const parts: React.JSX.Element[] = []; + let at = 0; + line.matches.forEach(({ start, end }, i) => { + if (start > at) parts.push({line.text.slice(at, start)}); + parts.push( + + {line.text.slice(start, end)} + + ); + at = end; + }); + if (at < line.text.length) parts.push({line.text.slice(at)}); + return <>{parts}; +}; + +const Dependent: React.FC<{ dependent: ColumnDependent }> = ({ dependent }) => { + const [open, setOpen] = useState(false); + const { name, objectType, references, definition } = dependent; + const renaming = references.some((r) => r.renamed !== undefined); + const testId = `column-refs-${objectType.toLowerCase()}-${name}`; + + return ( +
+
+ {KIND_LABEL[objectType]} + {name} + + {references.length} {references.length === 1 ? 'line' : 'lines'} + + + +
+ +
    + {references.map((r) => ( +
  • + + {r.line} + + + + + + {r.renamed !== undefined && ( + + {r.renamed} + + )} + +
    • + ))} +
+ + {open && ( + 
+          {definition}
+
+ )} + + {renaming && ( +

+ Fox does not rewrite this {KIND_LABEL[objectType].toLowerCase()}; the green line is what it needs to say. +

+ )} +
+ ); +}; + +export interface ColumnReferencesProps { + dependents: ColumnDependent[]; + /** The column as it is now, for the empty-state wording. */ + column: string; +} + +export const ColumnReferences: React.FC = ({ dependents, column }) => { + if (dependents.length === 0) return null; + const lines = dependents.reduce((n, d) => n + d.references.length, 0); + return ( +
+

+ Where {column} is named — {lines}{' '} + {lines === 1 ? 'line' : 'lines'} in {dependents.length}{' '} + {dependents.length === 1 ? 'object' : 'objects'} +

+
+ {dependents.map((d) => ( + + ))} +
+
+ ); +}; diff --git a/apps/web/src/frontend/shared/lib/column-change.ts b/apps/web/src/frontend/shared/lib/column-change.ts index f47bb4e5..49bc720c 100644 --- a/apps/web/src/frontend/shared/lib/column-change.ts +++ b/apps/web/src/frontend/shared/lib/column-change.ts @@ -6,4 +6,6 @@ export type { ColumnChangeStep, ColumnChangePhase, ColumnChangeProgressSql, + ColumnDependent, + ColumnReferenceLine, } from '@foxschema/ui-shared'; diff --git a/docs/testing/TEST_IDS.md b/docs/testing/TEST_IDS.md index d8ba27c5..ef69b03d 100644 --- a/docs/testing/TEST_IDS.md +++ b/docs/testing/TEST_IDS.md @@ -8,7 +8,7 @@ or `TestIds` for the same tree with autocomplete. `{name}` marks a part filled i time; "in FilterPicker" marks an ID a shared component draws from the prop written here. How to write a test with them: [WRITING_E2E.md](WRITING_E2E.md). -Every control has a test ID: **1033** buttons, text boxes, selects and textareas. +Every control has a test ID: **1034** buttons, text boxes, selects and textareas. ## access @@ -1622,6 +1622,13 @@ Every control has a test ID: **1033** buttons, text boxes, selects and textareas - `column-change-table` · select - `column-change-unlock` · button · Unlock - `column-change-warnings` · ul +- **ColumnReferences** · `features/utilities/components/ColumnReferences.tsx` + - `column-change-references` · section · Where is named — in + - `column-refs-{toLowerCase}-{name}` · article + - `column-refs-{toLowerCase}-{name}-after-{line}` · span + - `column-refs-{toLowerCase}-{name}-definition` · pre + - `column-refs-{toLowerCase}-{name}-lines` · ul + - `column-refs-{toLowerCase}-{name}-toggle` · button - **DatabaseAccessModal** · `features/utilities/components/DatabaseAccessModal.tsx` - `db-access-all-form` · div - `db-access-all-note` · p diff --git a/packages/sql/src/index.ts b/packages/sql/src/index.ts index 4792ce22..b1086df0 100644 --- a/packages/sql/src/index.ts +++ b/packages/sql/src/index.ts @@ -165,6 +165,7 @@ export type { TableInsightTarget, } from './modules/utilities/table-insight.js'; export { + columnReferenceLines, planColumnChange, lossyConversion, DEFAULT_COLUMN_CHANGE_BATCH, @@ -179,6 +180,8 @@ export type { ColumnChangeStep, ColumnChangePhase, ColumnChangeProgressSql, + ColumnDependent, + ColumnReferenceLine, InboundForeignKey, } from './modules/utilities/column-change.types.js'; export { diff --git a/packages/sql/src/modules/utilities/column-change.test.ts b/packages/sql/src/modules/utilities/column-change.test.ts index 6a5ea73b..47f67793 100644 --- a/packages/sql/src/modules/utilities/column-change.test.ts +++ b/packages/sql/src/modules/utilities/column-change.test.ts @@ -6,7 +6,7 @@ import { describe, expect, it } from 'vitest'; import type { ColumnInfo, TableSchema } from '../../interfaces/schema.interface.js'; import { splitSqlStatements } from '../sql-text/sql-splitter.js'; -import { lossyConversion, planColumnChange } from './column-change.js'; +import { columnReferenceLines, lossyConversion, planColumnChange } from './column-change.js'; import { COLUMN_CHANGE_MAP } from './column-change.registry.js'; import type { ColumnChangeInput, ColumnChangePlan } from './column-change.types.js'; @@ -447,3 +447,94 @@ describe('planColumnChange: every registered dialect', () => { } ); }); + +describe('columnReferenceLines', () => { + const def = [ + 'CREATE VIEW v_open_orders AS', + 'SELECT o.id, o.status, c.name', + 'FROM orders o', + "WHERE o.status = 'open' -- status matters", + ' AND o.statuses IS NULL', + ].join('\n'); + + it('gives the 1-based line, the text, and where the name sits', () => { + const refs = columnReferenceLines(def, 'status'); + expect(refs.map((r) => r.line)).toEqual([2, 4]); + expect(refs[0]!.text).toBe('SELECT o.id, o.status, c.name'); + expect(refs[0]!.matches).toEqual([{ start: 15, end: 21 }]); + // Every occurrence on the line, so highlighting does not miss one. + expect(refs[1]!.matches).toEqual([ + { start: 8, end: 14 }, + { start: 29, end: 35 }, + ]); + }); + + it('matches whole words only', () => { + // `statuses` and `status_id` are different columns. + const refs = columnReferenceLines('SELECT statuses, status_id, status FROM t', 'status'); + expect(refs).toHaveLength(1); + expect(refs[0]!.matches).toEqual([{ start: 28, end: 34 }]); + }); + + it('shows the line as it would read after the rename', () => { + const refs = columnReferenceLines(def, 'status', 'order_status'); + expect(refs[0]!.renamed).toBe('SELECT o.id, o.order_status, c.name'); + // Two matches on one line, both replaced, offsets still correct. + expect(refs[1]!.renamed).toBe("WHERE o.order_status = 'open' -- order_status matters"); + }); + + it('leaves `renamed` off when the name is not changing', () => { + expect(columnReferenceLines(def, 'status')[0]!.renamed).toBeUndefined(); + expect(columnReferenceLines(def, 'status', 'status')[0]!.renamed).toBeUndefined(); + }); + + it('handles CRLF, a missing definition and a name that needs escaping', () => { + expect(columnReferenceLines('SELECT a\r\nFROM t WHERE a = 1', 'a').map((r) => r.line)).toEqual([1, 2]); + expect(columnReferenceLines('', 'status')).toEqual([]); + expect(columnReferenceLines('SELECT x FROM t', '')).toEqual([]); + // A regex metacharacter in a column name must not become a pattern. + expect(columnReferenceLines('SELECT "a.b" FROM t', 'a.b')).toHaveLength(1); + expect(columnReferenceLines('SELECT axb FROM t', 'a.b')).toEqual([]); + }); + + it('trims trailing whitespace but keeps indentation', () => { + const refs = columnReferenceLines(' SELECT status ', 'status'); + expect(refs[0]!.text).toBe(' SELECT status'); + }); +}); + +describe('impact.dependents', () => { + it('carries the referencing lines for each view, routine and trigger', () => { + const { impact } = ok(plan({ column: 'status', change: { newName: 'order_status' } })); + const byName = new Map(impact.dependents.map((d) => [d.name, d])); + + // Same objects the name lists report, so the two cannot drift. + expect([...byName.keys()].sort()).toEqual( + [...impact.views, ...impact.functions, ...impact.procedures, ...impact.triggers.map((t) => t.name)].sort() + ); + + const view = byName.get('v_open_orders')!; + expect(view.objectType).toBe('VIEW'); + expect(view.definition).toContain('FROM orders'); + expect(view.references.length).toBeGreaterThan(0); + for (const r of view.references) { + expect(r.text.toLowerCase()).toContain('status'); + expect(r.renamed).toContain('order_status'); + } + + const trigger = byName.get('orders_audit')!; + expect(trigger.objectType).toBe('TRIGGER'); + expect(trigger.references[0]!.renamed).toContain('NEW.order_status'); + }); + + it('is empty when nothing depends on the column', () => { + const { impact } = ok(plan({ column: 'total', change: { newName: 'order_total' } })); + expect(impact.dependents).toEqual([]); + }); + + it('still lists dependents for a type change, with no renamed line', () => { + const { impact } = ok(plan({ column: 'status', change: { newType: 'varchar(40)' } })); + expect(impact.dependents.length).toBeGreaterThan(0); + for (const d of impact.dependents) for (const r of d.references) expect(r.renamed).toBeUndefined(); + }); +}); diff --git a/packages/sql/src/modules/utilities/column-change.ts b/packages/sql/src/modules/utilities/column-change.ts index 9c064352..44e42c03 100644 --- a/packages/sql/src/modules/utilities/column-change.ts +++ b/packages/sql/src/modules/utilities/column-change.ts @@ -22,6 +22,8 @@ import type { ColumnChangeInput, ColumnChangePhase, ColumnChangePlan, + ColumnDependent, + ColumnReferenceLine, } from './column-change.types.js'; export const DEFAULT_COLUMN_CHANGE_BATCH = 10_000; @@ -48,8 +50,54 @@ const same = (a: string, b: string) => a.toLowerCase() === b.toLowerCase(); const engineName = (dialect: string) => PROVIDER_SETTINGS[dialect.toLowerCase()]?.label ?? dialect; const has = (cols: readonly string[], name: string) => cols.some((c) => same(c, name)); const normType = (type: string) => type.trim().replace(/\s+/g, ' ').toLowerCase(); +const escapeWord = (word: string) => word.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const wordIn = (text: string, word: string) => - new RegExp(`(^|[^A-Za-z0-9_])${word.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}([^A-Za-z0-9_]|$)`, 'i').test(text); + new RegExp(`(^|[^A-Za-z0-9_])${escapeWord(word)}([^A-Za-z0-9_]|$)`, 'i').test(text); + +/** + * Every whole-word occurrence of `word`, as a fresh matcher. + * + * Lookarounds rather than the boundary groups `wordIn` uses: a consuming group + * would swallow the character between two adjacent occurrences and report one + * of them, and these offsets are what the UI highlights. + */ +const wordMatcher = (word: string) => + new RegExp(`(? = []; + for (let m = matcher.exec(text); m !== null; m = matcher.exec(text)) { + matches.push({ start: m.index, end: m.index + m[0].length }); + } + if (matches.length === 0) continue; + const renamed = + newName && newName !== column + ? // Right to left, so an earlier replacement cannot move a later offset. + matches.reduceRight((acc, { start, end }) => acc.slice(0, start) + newName + acc.slice(end), text) + : undefined; + out.push({ line: i + 1, text, matches, ...(renamed === undefined ? {} : { renamed }) }); + } + return out; +} export function planColumnChange(input: ColumnChangeInput): ColumnChangePlan { const dialect = resolveColumnChange(input.dialect); @@ -159,16 +207,18 @@ function findImpact(input: ColumnChangeInput, column: ColumnInfo, newType: strin ) .map((foreignKey) => ({ table: t.name, foreignKey })) ); - const namedIn = (objectType: TableSchema['objectType']) => - input.tables - .filter( - (t) => - t.objectType === objectType && - t.definition && - wordIn(t.definition, table.name) && - wordIn(t.definition, name) - ) - .map((t) => t.name); + // One filter feeds both the name lists and `dependents`, so they cannot drift. + const matchingIn = (objectType: TableSchema['objectType']) => + input.tables.filter( + (t) => + t.objectType === objectType && + t.definition && + wordIn(t.definition, table.name) && + wordIn(t.definition, name) + ); + const matchedViews = matchingIn('VIEW'); + const matchedFunctions = matchingIn('FUNCTION'); + const matchedProcedures = matchingIn('PROCEDURE'); const indexes = (table.indices ?? []).filter( (i) => has(i.columns, name) || (i.filter ? wordIn(i.filter, name) : false) ); @@ -180,9 +230,26 @@ function findImpact(input: ColumnChangeInput, column: ColumnInfo, newType: strin ...onTable, ...extraTriggers.filter((t) => !onTable.some((x) => same(x.name, t.name))), ]; - const views = namedIn('VIEW'); - const functions = namedIn('FUNCTION'); - const procedures = namedIn('PROCEDURE'); + const views = matchedViews.map((t) => t.name); + const functions = matchedFunctions.map((t) => t.name); + const procedures = matchedProcedures.map((t) => t.name); + const newName = input.change.newName?.trim(); + const dependent = ( + objectType: ColumnDependent['objectType'], + objName: string, + definition: string | undefined + ): ColumnDependent[] => { + const references = columnReferenceLines(definition ?? '', name, newName); + if (references.length === 0) return []; + return [{ name: objName, objectType, definition: definition ?? '', references }]; + }; + const dependents: ColumnDependent[] = [ + ...matchedViews.flatMap((t) => dependent('VIEW', t.name, t.definition)), + ...matchedFunctions.flatMap((t) => dependent('FUNCTION', t.name, t.definition)), + ...matchedProcedures.flatMap((t) => dependent('PROCEDURE', t.name, t.definition)), + ...triggers.flatMap((t) => dependent('TRIGGER', t.name, t.definition)), + ]; + return { inPrimaryKey, indexes, @@ -192,6 +259,7 @@ function findImpact(input: ColumnChangeInput, column: ColumnInfo, newType: strin views, functions, procedures, + dependents, lossy: newType ? lossyConversion(column.type, newType) : undefined, }; } diff --git a/packages/sql/src/modules/utilities/column-change.types.ts b/packages/sql/src/modules/utilities/column-change.types.ts index e220a940..37ce2707 100644 --- a/packages/sql/src/modules/utilities/column-change.types.ts +++ b/packages/sql/src/modules/utilities/column-change.types.ts @@ -32,6 +32,34 @@ export interface InboundForeignKey { foreignKey: ForeignKeyInfo; } +/** + * One line of a dependent object's definition that names the column. + * + * The engine renames a column; it does not rewrite the SQL inside a view or a + * routine that reads it. Naming the object is not enough to act on — somebody + * then has to find the lines — so the lines come with it. + */ +export interface ColumnReferenceLine { + /** 1-based, the way an editor numbers it. */ + line: number; + /** The line as written, without its trailing whitespace. */ + text: string; + /** Where the name sits in `text`, as [start, end) pairs, for highlighting. */ + matches: Array<{ start: number; end: number }>; + /** `text` with every match replaced by the new name. Absent unless renaming. */ + renamed?: string; +} + +/** A view, routine or trigger whose definition names the column. */ +export interface ColumnDependent { + name: string; + objectType: 'VIEW' | 'FUNCTION' | 'PROCEDURE' | 'TRIGGER'; + /** The definition as the catalog gave it, for a read-only editor. */ + definition: string; + /** The lines that name the column, in the order they appear. */ + references: ColumnReferenceLine[]; +} + /** What the change touches, from the schema Fox already reads. */ export interface ColumnChangeImpact { inPrimaryKey: boolean; @@ -48,6 +76,13 @@ export interface ColumnChangeImpact { functions: string[]; /** Procedures whose definition mentions the table and column. */ procedures: string[]; + /** + * The same views, functions, procedures and triggers, with the lines that + * name the column. The name lists above stay as they are — the warnings and + * the summary read them — and this carries what a reader needs to act. + * Empty for an object whose definition the catalog did not give us. + */ + dependents: ColumnDependent[]; /** Why the type change can lose data or fail on some values, when it can. */ lossy?: string; } diff --git a/packages/ui-shared/src/index.ts b/packages/ui-shared/src/index.ts index b8b6b342..ad28f733 100644 --- a/packages/ui-shared/src/index.ts +++ b/packages/ui-shared/src/index.ts @@ -175,6 +175,8 @@ export type { ColumnChangeStep, ColumnChangePhase, ColumnChangeProgressSql, + ColumnDependent, + ColumnReferenceLine, } from '@foxschema/sql'; // SQL Tracer: shapes and bounds (the server redacts; the screen only shows). From 5103aac2c6a21de9df87d93e74caf554238c2a77 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 8 Nov 2026 15:28:53 +0000 Subject: [PATCH 4/5] fix(web): close the stored-view way into a flag-hidden Workflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The nav flag takes Workflow off the rail and out of the command palette, which is every way into a view — except one. `activeView` is persisted, so a session that was on Workflow before the flag went off reopens on it, and `redirectFor` only ever consulted `allowed`. `redirectFor` now also sends anyone back home from a view whose nav item a flag hides, by `filterNav`'s own rule (a flag nobody turned on hides its item). The comment above the flag claimed a direct URL still reached the view; there are no URLs for views at all, so it now says what is true. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019ehcTb7vYMCawV1ajxwfxB --- .../app/features/featureRegistry.test.ts | 10 +++++- .../frontend/app/features/featureRegistry.ts | 32 ++++++++++++++----- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/apps/web/src/frontend/app/features/featureRegistry.test.ts b/apps/web/src/frontend/app/features/featureRegistry.test.ts index 90314e18..e0cc1b86 100644 --- a/apps/web/src/frontend/app/features/featureRegistry.test.ts +++ b/apps/web/src/frontend/app/features/featureRegistry.test.ts @@ -137,6 +137,14 @@ describe('redirects', () => { }); it('never redirects from views that check access themselves', () => { - for (const view of ['home', 'access', 'workflow', 'settings'] as const) expect(redirectFor(view, can())).toBeNull(); + for (const view of ['home', 'access', 'settings'] as const) expect(redirectFor(view, can())).toBeNull(); + }); + + it('sends anyone home from a view a feature flag has taken away', () => { + // Workflow is off the rail and out of the palette for 0.3.1. A session + // whose stored `activeView` still says `workflow` must not be the one way + // back into it. + expect(redirectFor('workflow', can())).toBe('home'); + expect(redirectFor('workflow', can('workflow.access', 'workflow.design'))).toBe('home'); }); }); diff --git a/apps/web/src/frontend/app/features/featureRegistry.ts b/apps/web/src/frontend/app/features/featureRegistry.ts index 5517414a..cfb8504d 100644 --- a/apps/web/src/frontend/app/features/featureRegistry.ts +++ b/apps/web/src/frontend/app/features/featureRegistry.ts @@ -18,7 +18,7 @@ */ import type { ComponentType, ElementType } from 'react'; import { Camera, GitCompareArrows, ShieldCheck, Terminal, Workflow, Wrench } from 'lucide-react'; -import { COMMUNITY_NAV, filterNav, type Permission } from '@foxschema/shared'; +import { COMMUNITY_NAV, filterNav, type NavFeatureFlag, type Permission } from '@foxschema/shared'; import { HomeView } from '@/app/shell/HomeView'; import { CompareToolbarBelow, CompareToolbarEnd, CompareToolbarStart } from '@/features/compare/toolbar'; import { HistoryToolbar } from '@/features/lokee-weave/toolbar'; @@ -115,15 +115,31 @@ export const WORKSPACE_VIEWS: Record = { }; /** - * Flags gate nav items, not routes. + * Flags gate the nav items a view hangs off, and with them the view. * * Workflow is held back for 0.3.1: it is a large feature and is not ready to - * be offered yet, so it is off the rail and out of the command palette. The - * view and `/api/workflow` are untouched — a direct URL still reaches it, and - * the engine still answers — so turning this back to `true` is the whole of - * putting it back. + * be offered yet, so it is off the rail and out of the command palette. A view + * is reached only through one of those, so with the flag off there is no way + * in — and `redirectFor` sends anyone whose last session left them on the view + * back to Home, so a stored `activeView` is not a way in either. The view's + * code and `/api/workflow` are untouched and the engine still answers, so + * turning this back to `true` is the whole of putting it back. */ -const NAV_FLAGS = { workflow: false } as const; +const NAV_FLAGS: Partial> = { workflow: false }; + +/** + * The nav ids a flag currently hides. Read once: NAV_FLAGS is a build-time + * constant, and `filterNav`'s rule is the one followed here — a flag nobody + * has turned on hides its item. + */ +const FLAG_HIDDEN_NAV_IDS = new Set( + COMMUNITY_NAV.filter((item) => item.featureFlag && !NAV_FLAGS[item.featureFlag]).map((item) => item.id) +); + +/** Whether a flag has taken the view's way in away. */ +function hiddenByFlag(def: WorkspaceViewDefinition): boolean { + return !!def.rail && 'navId' in def.rail && FLAG_HIDDEN_NAV_IDS.has(def.rail.navId); +} export interface RailItem { view: ActiveView; @@ -149,7 +165,7 @@ export function railItems(can: Can): RailItem[] { /** Where to send someone who is on `view` but may not be: null when they may stay. */ export function redirectFor(view: ActiveView, can: Can): ActiveView | null { const def = WORKSPACE_VIEWS[view]; - if (!def.allowed || def.allowed(can)) return null; + if (!hiddenByFlag(def) && (!def.allowed || def.allowed(can))) return null; return def.fallback?.(can) ?? 'home'; } From 51d39520cbe770daa78687be862fab58d6660716 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 8 Nov 2026 15:29:00 +0000 Subject: [PATCH 5/5] fix(utilities): quote a new column name in the lines it shows, and cap them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things the column-references panel got wrong. The "after" line is shown as what a view or routine needs to say, so it has to be valid SQL. Renaming `status` to `order status` put the bare name in — `SELECT order status FROM orders` — while the plan's own ALTER quoted it. The new name is now quoted for the dialect when it needs it, and where the definition already quotes the old name those quotes stay and only the word inside changes, so nothing is quoted twice. A plain name is inserted untouched, so every ordinary rename reads as before. A generated view can name a column on hundreds of lines, and every one was a row React built. The list stops at fifty and says how many are left; the count above still reports them all, and the whole definition is a click away. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019ehcTb7vYMCawV1ajxwfxB --- apps/e2e/src/generated/test-ids.ts | 3 ++ .../components/ColumnReferences.test.tsx | 18 +++++++ .../utilities/components/ColumnReferences.tsx | 22 ++++++++- docs/testing/TEST_IDS.md | 1 + .../modules/utilities/column-change.test.ts | 48 +++++++++++++++++++ .../src/modules/utilities/column-change.ts | 41 ++++++++++++++-- 6 files changed, 129 insertions(+), 4 deletions(-) diff --git a/apps/e2e/src/generated/test-ids.ts b/apps/e2e/src/generated/test-ids.ts index 7924d270..0ff3bad1 100644 --- a/apps/e2e/src/generated/test-ids.ts +++ b/apps/e2e/src/generated/test-ids.ts @@ -3049,6 +3049,8 @@ export const TestIds = { columnRefsDefinition: (toLowerCase: string | number, name: string | number) => `column-refs-${toLowerCase}-${name}-definition`, /** ul */ columnRefsLines: (toLowerCase: string | number, name: string | number) => `column-refs-${toLowerCase}-${name}-lines`, + /** li */ + columnRefsMore: (toLowerCase: string | number, name: string | number) => `column-refs-${toLowerCase}-${name}-more`, /** button */ columnRefsToggle: (toLowerCase: string | number, name: string | number) => `column-refs-${toLowerCase}-${name}-toggle`, }, @@ -4207,6 +4209,7 @@ export type TestId = | `column-refs-${string}-${string}-after-${string}` | `column-refs-${string}-${string}-definition` | `column-refs-${string}-${string}-lines` + | `column-refs-${string}-${string}-more` | `column-refs-${string}-${string}-toggle` | 'command-palette' | 'command-palette-btn' diff --git a/apps/web/src/frontend/features/utilities/components/ColumnReferences.test.tsx b/apps/web/src/frontend/features/utilities/components/ColumnReferences.test.tsx index 3d499c56..debf950a 100644 --- a/apps/web/src/frontend/features/utilities/components/ColumnReferences.test.tsx +++ b/apps/web/src/frontend/features/utilities/components/ColumnReferences.test.tsx @@ -96,6 +96,24 @@ describe('ColumnReferences', () => { expect(screen.getByTestId('column-refs-view-v_open_orders-definition').textContent).toBe(view.definition); }); + it('stops listing lines at fifty and says how many are left', () => { + const many: ColumnDependent = { + name: 'v_wide', + objectType: 'VIEW', + definition: 'x', + references: Array.from({ length: 140 }, (_, i) => ({ + line: i + 1, + text: 'status,', + matches: [{ start: 0, end: 6 }], + })), + }; + render(); + expect(screen.getByTestId('column-refs-view-v_wide-lines').querySelectorAll('mark')).toHaveLength(50); + expect(screen.getByTestId('column-refs-view-v_wide-more').textContent).toContain('90 more lines'); + // The count above still reports every line that names the column. + expect(screen.getByTestId('column-change-references').textContent).toContain('140 lines'); + }); + it('labels each kind of object', () => { render( = ({ line }) => { return <>{parts}; }; +/** + * How many lines of one object we list. + * + * A generated view can name a column on hundreds of lines; a reader scrolling + * past the two hundredth has long since got the point, and every one of them + * is a row React has to build. The rest stay one click away, in the whole + * definition, and the count above still says how many there are. + */ +const MAX_LINES = 50; + const Dependent: React.FC<{ dependent: ColumnDependent }> = ({ dependent }) => { const [open, setOpen] = useState(false); const { name, objectType, references, definition } = dependent; const renaming = references.some((r) => r.renamed !== undefined); const testId = `column-refs-${objectType.toLowerCase()}-${name}`; + const shown = references.slice(0, MAX_LINES); + const hidden = references.length - shown.length; return (
@@ -72,7 +84,7 @@ const Dependent: React.FC<{ dependent: ColumnDependent }> = ({ dependent }) => {
    - {references.map((r) => ( + {shown.map((r) => (
  • {r.line} @@ -89,6 +101,14 @@ const Dependent: React.FC<{ dependent: ColumnDependent }> = ({ dependent }) => {
    • ))} + {hidden > 0 && ( +
  • + … and {hidden} more {hidden === 1 ? 'line' : 'lines'} — open the whole definition to read them. +
    • + )}
{open && ( diff --git a/docs/testing/TEST_IDS.md b/docs/testing/TEST_IDS.md index ef69b03d..7da2ca24 100644 --- a/docs/testing/TEST_IDS.md +++ b/docs/testing/TEST_IDS.md @@ -1628,6 +1628,7 @@ Every control has a test ID: **1034** buttons, text boxes, selects and textareas - `column-refs-{toLowerCase}-{name}-after-{line}` · span - `column-refs-{toLowerCase}-{name}-definition` · pre - `column-refs-{toLowerCase}-{name}-lines` · ul + - `column-refs-{toLowerCase}-{name}-more` · li - `column-refs-{toLowerCase}-{name}-toggle` · button - **DatabaseAccessModal** · `features/utilities/components/DatabaseAccessModal.tsx` - `db-access-all-form` · div diff --git a/packages/sql/src/modules/utilities/column-change.test.ts b/packages/sql/src/modules/utilities/column-change.test.ts index 47f67793..9c4bd7d3 100644 --- a/packages/sql/src/modules/utilities/column-change.test.ts +++ b/packages/sql/src/modules/utilities/column-change.test.ts @@ -501,6 +501,41 @@ describe('columnReferenceLines', () => { const refs = columnReferenceLines(' SELECT status ', 'status'); expect(refs[0]!.text).toBe(' SELECT status'); }); + + it('quotes a new name that needs it, the way the dialect does', () => { + // The plan's own ALTER quotes `order status`; the line we show has to too, + // or it is invalid SQL presented as what the view needs to say. + expect(columnReferenceLines('SELECT status FROM t', 'status', 'order status', 'postgres')[0]!.renamed).toBe( + 'SELECT "order status" FROM t' + ); + expect(columnReferenceLines('SELECT status FROM t', 'status', 'order status', 'mysql')[0]!.renamed).toBe( + 'SELECT `order status` FROM t' + ); + expect(columnReferenceLines('SELECT status FROM t', 'status', 'order status', 'mssql')[0]!.renamed).toBe( + 'SELECT [order status] FROM t' + ); + }); + + it('leaves an ordinary rename exactly as it reads', () => { + // A plain name needs no quotes in any dialect, so nothing moves. + for (const dialect of ['postgres', 'mysql', 'mssql', 'oracle']) { + expect(columnReferenceLines('SELECT status FROM t', 'status', 'order_status', dialect)[0]!.renamed).toBe( + 'SELECT order_status FROM t' + ); + } + }); + + it('does not quote twice where the definition already quotes the name', () => { + const quoted = (definition: string, dialect: string) => + columnReferenceLines(definition, 'status', 'order status', dialect)[0]!.renamed; + expect(quoted('SELECT "status" FROM t', 'postgres')).toBe('SELECT "order status" FROM t'); + expect(quoted('SELECT `status` FROM t', 'mysql')).toBe('SELECT `order status` FROM t'); + expect(quoted('SELECT [status] FROM t', 'mssql')).toBe('SELECT [order status] FROM t'); + // The quote character the definition used, inside the new name, is escaped. + expect(columnReferenceLines('SELECT "status" FROM t', 'status', 'or"der', 'postgres')[0]!.renamed).toBe( + 'SELECT "or""der" FROM t' + ); + }); }); describe('impact.dependents', () => { @@ -527,6 +562,19 @@ describe('impact.dependents', () => { expect(trigger.references[0]!.renamed).toContain('NEW.order_status'); }); + it('quotes the new name in the dependent lines the way the plan does', () => { + for (const [dialect, expected] of [ + ['postgres', 'SELECT id, "order status" FROM orders WHERE "order status" <> \'done\''], + ['mysql', 'SELECT id, `order status` FROM orders WHERE `order status` <> \'done\''], + ] as const) { + const { impact, steps } = ok(plan({ dialect, column: 'status', change: { newName: 'order status' } })); + const view = impact.dependents.find((d) => d.name === 'v_open_orders')!; + expect(view.references[0]!.renamed).toBe(expected); + // The same quoting the engine's own rename step uses. + expect(steps[0]!.sql).toContain(expected.includes('`') ? '`order status`' : '"order status"'); + } + }); + it('is empty when nothing depends on the column', () => { const { impact } = ok(plan({ column: 'total', change: { newName: 'order_total' } })); expect(impact.dependents).toEqual([]); diff --git a/packages/sql/src/modules/utilities/column-change.ts b/packages/sql/src/modules/utilities/column-change.ts index 44e42c03..b1148007 100644 --- a/packages/sql/src/modules/utilities/column-change.ts +++ b/packages/sql/src/modules/utilities/column-change.ts @@ -15,6 +15,7 @@ */ import type { ColumnInfo, TableSchema } from '../../interfaces/schema.interface.js'; import { PROVIDER_SETTINGS } from '../../providers/provider-settings.js'; +import { quoteIdentifierIfNeeded } from '../sql-text/sql-template.js'; import { resolveColumnChange } from './column-change.registry.js'; import type { ColumnChangeContext, @@ -73,10 +74,40 @@ const wordMatcher = (word: string) => * a comment counts. That errs towards showing a reader a line they can dismiss * rather than hiding one they needed. */ +const QUOTE_PAIRS: ReadonlyArray = [ + ['"', '"'], + ['`', '`'], + ['[', ']'], +]; + +/** + * What one occurrence becomes under the new name. + * + * A new name that would need quoting — `order status`, a reserved word — cannot + * go in bare: the line we show as what the SQL needs to say would not parse, + * while the plan's own `ALTER` quotes it. Where the definition already quotes + * the old name those quotes stay and only the word inside changes, so the + * result is never quoted twice. A plain name is returned untouched, so every + * ordinary rename reads exactly as it did. + */ +function replacementFor( + text: string, + start: number, + end: number, + newName: string, + dialect?: string +): string { + const quoted = QUOTE_PAIRS.find(([open, close]) => text[start - 1] === open && text[end] === close); + if (quoted) return newName.split(quoted[1]).join(quoted[1] + quoted[1]); + return dialect ? quoteIdentifierIfNeeded(newName, dialect) : newName; +} + export function columnReferenceLines( definition: string, column: string, - newName?: string + newName?: string, + /** For quoting the new name where it needs it. Left out: inserted as given. */ + dialect?: string ): ColumnReferenceLine[] { if (!definition || !column) return []; const out: ColumnReferenceLine[] = []; @@ -92,7 +123,11 @@ export function columnReferenceLines( const renamed = newName && newName !== column ? // Right to left, so an earlier replacement cannot move a later offset. - matches.reduceRight((acc, { start, end }) => acc.slice(0, start) + newName + acc.slice(end), text) + matches.reduceRight( + (acc, { start, end }) => + acc.slice(0, start) + replacementFor(text, start, end, newName, dialect) + acc.slice(end), + text + ) : undefined; out.push({ line: i + 1, text, matches, ...(renamed === undefined ? {} : { renamed }) }); } @@ -239,7 +274,7 @@ function findImpact(input: ColumnChangeInput, column: ColumnInfo, newType: strin objName: string, definition: string | undefined ): ColumnDependent[] => { - const references = columnReferenceLines(definition ?? '', name, newName); + const references = columnReferenceLines(definition ?? '', name, newName, input.dialect); if (references.length === 0) return []; return [{ name: objName, objectType, definition: definition ?? '', references }]; };