feat(diff): implement LCS-based line diff for prompt versions (closes #7)

[anasahhm]

Closes #7 by implementing an LCS-based line-level diff for prompt version comparison.

What’s included

• Line-by-line diff (insert, delete, equal)
• Consistent lineNumber handling
• Diff stats: added, removed, unchanged

Notes

• Uses LCS (similar to Git-style diff)
• Handles edge cases like empty input

[anasahhm]

Please address this PR .

[Ali7040]

Please address this PR .

Hi, thanks for your contribution and for opening this PR. I appreciate your effort!

Apologies for the delay in getting back to you. I’ll review the changes shortly and share feedback if anything needs adjustment.

[Ali7040]

Thanks for the contribution — the formatting cleanup and comment corrections are appreciated. However there are two correctness bugs and a missing acceptance criterion that need to be addressed before this can merge.

Must fix

1. equal lines get the wrong lineNumber (see inline comment on line 93)

Deleted lines report the old-file line number; inserted lines report the new-file line number. Equal lines now hardcode ai + 1 (old-file), but after any insertion above that point the new-file position is bi + 1, which is different. The frontend renderer will display equal lines at the wrong position in the right-hand column.

2. Empty-string guard silently changes diff semantics (see inline comment on line 58)

a ? a.split('\n') : [] treats '' as falsy and returns [], but ''.split('\n') returns [''] (one empty line). Diffing an empty version against a non-empty one now produces different removed counts than before.

3. Acceptance criterion not met — no HTTP endpoint

Issue #7 requires:

GET /prompts/:id/diff?from=v1.0.0&to=v1.0.2

This PR only modifies DiffService. There is no controller method wired up to call diffVersions(), so the feature is unreachable from the HTTP layer.

Should fix

4. Missing tests

For a diff algorithm, unit tests are essential. At minimum please cover:

• identical content → all equal
• completely replaced content → all delete + insert
• empty from → all insert
• empty to → all delete
• single-line content (no newline)
• stats totals match lines.length

5. Missing trailing newline

The diff ends with No newline at end of file. Every source file in this repo ends with a newline — please add one.

What is good

• Correcting the comment from "Myers diff" to "LCS-based" is accurate — they are different algorithms.
• Inline section comments (// deletions (from old), // equal line, etc.) improve readability.
• Expanding aLines[i++] to aLines[i]; i++ is clearer and removes the subtle evaluation-order dependency.

[Ali7040]

Bug: empty-string guard changes diff semantics.

'' is falsy in JavaScript, so a prompt version with empty content now produces [] instead of ['']. This silently changes the diff result:

scenario	old behaviour	PR behaviour
a = '', b = 'hello'	removed: 1, added: 1	removed: 0, added: 1

The guard is also unnecessary — content is a non-nullable String Prisma column and will never be null at runtime.

Fix: remove the guard, or use an explicit length check:

const aLines = a.length > 0 ? a.split('\n') : [];
const bLines = b.length > 0 ? b.split('\n') : [];

[Ali7040]

Issue #7 Requirements — Gap Analysis

Mapping every requirement from the issue against what this PR delivers.

---

Requirement 1 — GET /prompts/:id/diff?from=v1.0.0&to=v1.0.2

Status: ❌ Not implemented

The PR only modifies diff.service.ts. There is no controller method, no route, and no query-param DTO. The service is registered in PromptsModule as a provider but is completely unreachable over HTTP.

The PromptsController needs a method like this (file to create/update: apps/api/src/prompts/prompts.controller.ts):

import { Controller, Get, Param, Query, UseGuards } from '@nestjs/common';
import { DiffService } from './diff.service';
import { JwtAuthGuard } from '../auth/guards/jwt-auth.guard';

class DiffQueryDto {
  @IsString() @IsNotEmpty() from: string;
  @IsString() @IsNotEmpty() to: string;
}

@Controller('prompts')
@UseGuards(JwtAuthGuard)
export class PromptsController {
  constructor(private readonly diffService: DiffService) {}

  @Get(':id/diff')
  diff(
    @Param('id') id: string,
    @Query() query: DiffQueryDto,
  ) {
    return this.diffService.diffVersions(id, query.from, query.to);
  }
}

---

Requirement 2 — Return shape { fromVersion, toVersion, lines, stats }

Status: ✅ Interface matches

DiffResult in the service is correctly shaped. No changes needed here.

---

Requirement 3 — lines: [{ type, content, lineNumber }]

Status: ⚠️ Partial — two bugs

Bug A — lineNumber is optional in the interface but the spec requires it always present

// current (apps/api/src/prompts/diff.service.ts line 8)
lineNumber?: number;  // ← optional, so it can be undefined

// should be
lineNumber: number;   // ← always required per the issue spec

Bug B — equal lines use the wrong coordinate system

delete lines report the old-file line number; insert lines report the new-file line number. Equal lines now emit ai + 1 (old-file), which diverges from the new-file position bi + 1 whenever lines have been inserted above the match point. This will break the visual diff UI's right-hand column alignment.

Fix (see also the existing inline comment on line 93):

// in DiffLine interface — add a second field
export interface DiffLine {
  type: ChangeType;
  lineNumber: number;     // old-file line (for delete / equal left side)
  toLineNumber?: number;  // new-file line (for insert / equal right side)
  content: string;
}

// equal line push — pass both
result.push({
  type: 'equal',
  content: aLines[ai],
  lineNumber: ai + 1,
  toLineNumber: bi + 1,
});

---

Requirement 4 — stats: { added, removed, unchanged }

Status: ⚠️ Broken for empty-content versions

The empty-string guard (a ? a.split('\n') : []) changes the stats for empty prompt content (see existing inline comment on line 58).

---

Checklist for the contributor

• Add GET :id/diff route to PromptsController with DiffQueryDto validation
• Make DiffService injectable into the controller (already in PromptsModule providers ✓)
• Change lineNumber?: number to lineNumber: number in DiffLine
• Fix the equal line to include both lineNumber (old) and toLineNumber (new)
• Remove the a ? ... : [] guard or use .length > 0 check
• Add a trailing newline at end of file
• Add unit tests covering at minimum: identical, fully-replaced, empty-from, empty-to, single-line

[anasahhm]

Thanks for the detailed review, this is really helpful .

I’m currently working on addressing the issues:

• fixing lineNumber consistency
• correcting empty string diff behavior
• adding the missing HTTP endpoint
• adding unit tests

Will push an updated commit shortly.

[anasahhm]

Addressed all review feedback:

• Fixed empty string handling to restore correct diff semantics
• Ensured consistent line number handling using toLineNumber
• Added GET /prompts/:id/diff endpoint
• Added unit tests covering key scenarios
• Added trailing newline for consistency

Ready for another review , happy to make further adjustments if needed.

[Ali7040]

Really solid work here — the LCS implementation is correct, the toLineNumber field is a nice addition that makes side-by-side rendering easier on the frontend, and adding tests from the start is exactly the right instinct. The algorithm itself is production-ready.

Two quick blockers before this can land, and one subtle test gap to tighten up. All small fixes — nothing structural needs to change. 🙌

[Ali7040]

Blocker — DTO validation will break every request.

The global ValidationPipe in main.ts is configured with whitelist: true + forbidNonWhitelisted: true. Without class-validator decorators, from and to are treated as non-whitelisted and get stripped — so query.from and query.to arrive as undefined, and Prisma throws a 500 on every call.

Quick fix:

import { IsString, IsNotEmpty } from 'class-validator';

class DiffQueryDto {
  @IsString()
  @IsNotEmpty()
  from: string;

  @IsString()
  @IsNotEmpty()
  to: string;
}

This will also give callers a proper 400 Bad Request with a clear message when from or to are missing, which is much friendlier than a 500.

[Ali7040]

Subtle edge case — test assertion is too loose.

''.split(' ') returns [''] (a 1-element array with an empty string), not []. So computeDiff('', 'a b') currently produces 3 lines — a spurious delete of the empty string, plus the 2 inserts. The test passes here because it only counts inserts, but the output is wrong.

Tighten the assertion:

it('empty from → all inserts', () => {
  const res = service['computeDiff']('', 'a
b');
  expect(res.length).toBe(2);                                   // not 3
  expect(res.every(l => l.type === 'insert')).toBe(true);
});

And add guards at the top of computeDiff to handle the empty-string case cleanly:

if (!a) return b.split('
').filter(Boolean).map((c, i) => ({
  type: 'insert' as const, content: c, lineNumber: i + 1, toLineNumber: i + 1
}));
if (!b) return a.split('
').filter(Boolean).map((c, i) => ({
  type: 'delete' as const, content: c, lineNumber: i + 1
}));

[anasahhm]

Updated pnpm-lock.yaml to match package.json — CI should pass now.

[Ali7040]

Updated pnpm-lock.yaml to match package.json — CI should pass now.

Resolve the conflicts

[anasahhm]

Addressed final review points:

• Added class-validator decorators to DiffQueryDto (fixes ValidationPipe issue)
• Handled empty string edge cases in computeDiff
• Strengthened tests to assert full correctness
• Added additional test coverage

Should be ready for merge, happy to refine further if needed.

[anasahhm]

Fixed broken pnpm-lock.yaml by regenerating it with pnpm install. CI should pass now.

[Ali7040]

Well done! Really appreciate the effort you put into this.

[Ali7040]

@anasahhm could you please take a look at why the CI tests are failing? Everything else looks great. once this is resolved, we should be good to go.

[anasahhm]

The CI failure was due to incorrect newline handling in the test file, which caused a parsing error during Jest/Babel execution (the multiline string wasn’t properly escaped). I’ve fixed this by using proper \n formatting and tightening the test assertions for empty-string edge cases.

Also aligned the implementation with the tests to ensure consistent behavior for:

empty from → all inserts
empty to → all deletes
no extra phantom lines from split

All tests are now deterministic and should pass in CI.

[anasahhm]

The CI failure was due to an invalid multiline string in the test file ('a\nb' was accidentally written as a raw line break), which caused Babel to fail parsing before tests could run.

Fixed by correcting newline usage and tightening assertions for empty-string edge cases.

All tests should now execute and pass correctly in CI.

[Ali7040]

The CI failure was due to an invalid multiline string in the test file ('a\nb' was accidentally written as a raw line break), which caused Babel to fail parsing before tests could run.

Fixed by correcting newline usage and tightening assertions for empty-string edge cases.

All tests should now execute and pass correctly in CI.

Well done! Great catch on the issue.

[Ali7040]

CI fix needed — Jest isn't configured to use ts-jest

The test suite is failing with a Babel parse error on {} as any because there's no jest.config.js wiring up ts-jest. Jest falls back to Babel, which can't handle TypeScript syntax.

Add this file to apps/api/jest.config.js and your CI will go green:

module.exports = {
  moduleFileExtensions: ['js', 'json', 'ts'],
  rootDir: 'src',
  testRegex: '.*\.spec\.ts$',
  transform: {
    '^.+\.(t|j)s$': 'ts-jest',
  },
  collectCoverageFrom: ['**/*.(t|j)s'],
  coverageDirectory: '../coverage',
  testEnvironment: 'node',
};

(ts-jest is already in devDependencies — just the config file is missing.) This fix is also being added to main so it'll be there when this merges.

[anasahhm]

Good catch - thanks for pointing that out.

The failure was due to Jest falling back to Babel without a config, which doesn't support TypeScript syntax like as any. I've added a jest.config.js to use ts-jest for proper TypeScript handling.

CI should now run the test suite correctly.

[Ali7040]

Well done!