Skip to content

Comments

Promote test to main#59

Merged
ty2k merged 40 commits intomainfrom
test
May 16, 2025
Merged

Promote test to main#59
ty2k merged 40 commits intomainfrom
test

Conversation

@ty2k
Copy link
Collaborator

@ty2k ty2k commented May 15, 2025

No description provided.

ty2k added 25 commits May 12, 2025 09:08
Build graphemes-api Postgres connection string from multiple environment variables
Filter empty strings from database before sending API response
@ty2k ty2k requested a review from Copilot May 15, 2025 21:08
@ty2k ty2k self-assigned this May 15, 2025
@ty2k ty2k added the promotion For PRs that promote code from one environment to another label May 15, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds string-splitting utility with tests, implements a new languages API (v1) backed by Zod schema transformations, and configures database connectivity alongside deployment setup.

  • Introduces splitCommaSeparatedString helper and corresponding Vitest suite
  • Defines LanguageSchema and exposes GET endpoints for /api/v1/languages and /api/v1/languages/:id
  • Configures Knex via environment variables, integrates routers, loads .env, and adds OpenShift deployment manifest

Reviewed Changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
utils/splitCommaSeparatedString.ts Add utility to split and trim comma-separated text
utils/splitCommaSeparatedString.test.ts Add Vitest cases for utility behavior
routes/api/v1/languages/index.ts Add GET all and GET by ID for languages
routes/api/v1/index.ts Register /languages under v1 router
routes/api/index.ts Mount v1 router and remove default /api endpoint
models/Language.ts Define Zod schema with transforms
index.ts Load dotenv before starting Express
db/knexfile.ts Introduce Knex configuration via env vars
db/index.ts Initialize and export Knex instance
openshift/dev/deployment.yaml Add OpenShift development deployment manifest
Files not reviewed (1)
  • graphemes-api/package.json: Language not supported
Comments suppressed due to low confidence (2)

graphemes-api/src/utils/splitCommaSeparatedString.ts:10

  • There is no test case covering a whitespace-only input (e.g., " "), which triggers the early return. Consider adding a test to validate that whitespace-only strings return an empty array.
if (!str.trim()) {

graphemes-api/src/routes/api/index.ts:4

  • [nitpick] Removing the default GET /api route removes a basic endpoint that clients may rely on for a health-check or welcome message; consider reintroducing a minimal route or documenting this breaking change.
// GET /api

@ty2k ty2k requested a review from Copilot May 16, 2025 00:13
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Promote test branch changes into main, adding a new string-splitting utility, full language routes, rate limiting, and database configuration.

  • Introduce splitCommaSeparatedString util with comprehensive tests
  • Add /api/v1/languages endpoints with Zod-based parsing
  • Configure rate limiting, Knex setup, and deployment manifests

Reviewed Changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/utils/splitCommaSeparatedString.ts New helper to split and trim comma-separated text
src/utils/splitCommaSeparatedString.test.ts Tests covering whitespace, empty segments, and grapheme handling
src/routes/api/v1/languages/index.ts GET handlers for all languages and by ID, with Zod validation
src/routes/api/v1/index.ts Mount /languages subrouter under /api/v1
src/routes/api/index.ts Switch from a root /api GET to versioned routing (/v1)
src/models/Language.ts Zod schema using splitCommaSeparatedString transforms
src/index.ts Add global rate limiting middleware
src/db/knexfile.ts Knex configuration with environment warnings
src/db/index.ts Initialize Knex client based on NODE_ENV
openshift/dev/deployment.yaml Deployment manifest with environment variables for database
Files not reviewed (1)
  • graphemes-api/package.json: Language not supported
Comments suppressed due to low confidence (2)

graphemes-api/src/routes/api/index.ts:5

  • The root GET /api endpoint was removed, which may break clients expecting a response at /api. Consider reintroducing a default route or redirect to /api/v1 to maintain backward compatibility.
const apiRouter = Router();

graphemes-api/src/models/Language.ts:10

  • Using z.string().transform(...) assumes the database fields are always strings. If any field can be null or undefined, this will throw at runtime—consider using z.string().nullable() or a preprocess to handle missing values before transforming.
.transform(splitCommaSeparatedString)

# This is a literal value because `crunchy-postgres`
# is the default database name in the PostgresCluster.
# This database name does not appear in the Secret.
value: grapheme_v2
Copy link

Copilot AI May 16, 2025

Choose a reason for hiding this comment

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

[nitpick] Hardcoding the database name in the manifest can lead to drift and exposes configuration details. Consider sourcing DB_NAME from a secret or ConfigMap to centralize environment configuration.

Copilot uses AI. Check for mistakes.
@ty2k ty2k merged commit 2845f0b into main May 16, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

promotion For PRs that promote code from one environment to another

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant