Skip to content

Conversation

FelixMalfait
Copy link
Member

This probably comes too late for 0.43? I had this in my stash and forgot to raise a PR before leaving last week.
I can update it to 0.44 if it's too late

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR adds workspace versioning functionality by introducing a version field for workspaces and a migration command to backfill existing workspaces with version data.

  • Critical: Version field missing from insert columns in workspaces.ts seed data, causing version data to be lost during insertion
  • Brittle path resolution in version.util.ts using relative path ../../../../package.json could break if file structure changes
  • getAppVersion() has performance concerns with synchronous file reads and lacks caching
  • Migration command may need to be updated from 0.43 to 0.44 based on timing
  • Consider adding version string validation in version.util.ts to ensure proper format

5 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile

import { Module } from '@nestjs/common';
import { TypeOrmModule } from '@nestjs/typeorm';

import { MigrationCommandModule } from 'src/database/commands/migration-command/miration-command.module';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax: Typo in import path: 'miration-command.module' should be 'migration-command.module'

Suggested change
import { MigrationCommandModule } from 'src/database/commands/migration-command/miration-command.module';
import { MigrationCommandModule } from 'src/database/commands/migration-command/migration-command.module';

Comment on lines +41 to +43
this.logger.log(
chalk.red(`Error in workspace ${workspaceId} - ${error.message}`),
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Error is only logged but not propagated. Could lead to silent failures in the migration process.

Suggested change
this.logger.log(
chalk.red(`Error in workspace ${workspaceId} - ${error.message}`),
);
this.logger.log(
chalk.red(`Error in workspace ${workspaceId} - ${error.message}`),
);
throw error;

Comment on lines +7 to +9
const packageJson = JSON.parse(readFileSync(packageJsonPath, 'utf8'));

return packageJson.version;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: No validation that packageJson.version exists or is in valid semver format

Suggested change
const packageJson = JSON.parse(readFileSync(packageJsonPath, 'utf8'));
return packageJson.version;
const packageJson = JSON.parse(readFileSync(packageJsonPath, 'utf8'));
return typeof packageJson.version === 'string' ? packageJson.version : null;

@FelixMalfait FelixMalfait deleted the backfill-workspace-version-column branch August 27, 2025 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant