feat(exercise): English-only exercise data - #90
Conversation
46084bd to
7f33432
Compare
Exercise records carried paired Persian and English columns for name,
description, tips and mistakes, and `Program`/`ProgramDay` carried paired names
too. The catalog makes the pairing redundant and expensive: all 649 of its
entries would need both languages written and maintained by hand, and the
upstream source has only English.
The app's *interface* stays bilingual. Buttons, labels and screens still switch
to Persian from `messages/`, Persian is still the default locale, and RTL layout
is untouched. Only content collapses.
Columns are dropped rather than merged - there is no data to preserve.
Two things fall out of this that are worth more than the column change itself.
The name resolver collapses to a single `findUnique`. It used to be a two-step
dance: `nameFa` by unique key, then `nameEn` with an explicit `orderBy: { id:
"asc" }`, because `nameEn` was not unique and without that tie-break a program
could silently rebind to a different exercise between uploads (#45). One name
under `@@unique([userId, name])` removes the ambiguity that bug lived in, so the
tie-break has nothing left to protect.
`AmbiguousExerciseError` is deleted outright. It existed because a non-unique
`nameEn` could match several rows and the strict resolver had to refuse rather
than quietly report the wrong lift's numbers. A name now matches at most one
row, so the situation cannot arise. The tests that described it are rewritten to
assert the constraint that replaced it.
`InfoPanel` loses its `locale` prop, which existed only to choose a language per
field.
Verified in the running app, not only in tests: uploaded the real program YAML,
then checked that Persian UI chrome still renders, `dir="rtl"` is intact, and
English exercise names sit correctly inside the Persian page with no `undefined`
or `[object Object]` reaching the markup.
7f33432 to
784d0f1
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| DROP COLUMN "tipsFa", | ||
| ADD COLUMN "description" TEXT NOT NULL DEFAULT '', | ||
| ADD COLUMN "mistakes" TEXT[] DEFAULT ARRAY[]::TEXT[], | ||
| ADD COLUMN "name" TEXT NOT NULL, |
There was a problem hiding this comment.
CRITICAL: Adding name as TEXT NOT NULL without DEFAULT will fail on populated tables
After dropping nameFa/nameEn, existing rows would receive NULL for the new column, violating the NOT NULL constraint. Add a DEFAULT '' or make the column nullable first, populate it, then set NOT NULL.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| -- AlterTable | ||
| ALTER TABLE "Program" DROP COLUMN "nameEn", | ||
| DROP COLUMN "nameFa", | ||
| ADD COLUMN "name" TEXT NOT NULL; |
There was a problem hiding this comment.
CRITICAL: Adding name as TEXT NOT NULL without DEFAULT will fail on populated tables
After dropping nameFa/nameEn, existing rows would receive NULL for the new column, violating the NOT NULL constraint. Add a DEFAULT '' or make the column nullable first, populate it, then set NOT NULL.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| -- AlterTable | ||
| ALTER TABLE "ProgramDay" DROP COLUMN "nameEn", | ||
| DROP COLUMN "nameFa", | ||
| ADD COLUMN "name" TEXT NOT NULL; |
There was a problem hiding this comment.
CRITICAL: Adding name as TEXT NOT NULL without DEFAULT will fail on populated tables
After dropping nameFa/nameEn, existing rows would receive NULL for the new column, violating the NOT NULL constraint. Add a DEFAULT '' or make the column nullable first, populate it, then set NOT NULL.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| { nameEn: { contains: needle, mode: "insensitive" } }, | ||
| { nameFa: { contains: needle, mode: "insensitive" } }, | ||
| { name: { contains: needle, mode: "insensitive" } }, | ||
| { name: { contains: needle, mode: "insensitive" } }, |
There was a problem hiding this comment.
WARNING: Duplicate condition in OR array - second entry is identical to the first
Both nameEn and nameFa were collapsed into a single name field, but both OR conditions were kept. The second condition is redundant.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| { nameEn: { contains: longestWord, mode: "insensitive" } }, | ||
| { nameFa: { contains: longestWord, mode: "insensitive" } }, | ||
| { name: { contains: longestWord, mode: "insensitive" } }, | ||
| { name: { contains: longestWord, mode: "insensitive" } }, |
There was a problem hiding this comment.
WARNING: Duplicate condition in OR array - second entry is identical to the first
Both nameEn and nameFa were collapsed into a single name field, but both OR conditions were kept. The second condition is redundant.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| { nameEn: { contains: search, mode: "insensitive" } }, | ||
| { nameFa: { contains: search, mode: "insensitive" } }, | ||
| { name: { contains: search, mode: "insensitive" } }, | ||
| { name: { contains: search, mode: "insensitive" } }, |
There was a problem hiding this comment.
WARNING: Duplicate condition in OR array - second entry is identical to the first
Both nameEn and nameFa were collapsed into a single name field, but both OR conditions were kept. The second condition is redundant.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (30 files)
Fix these issues in Kilo Cloud Reviewed by free · Input: 149.3K · Output: 24.5K · Cached: 1.6M |
1 participant
Closes #75
Why
Exercise records carried paired Persian and English columns for name,
description, tips and mistakes;
ProgramandProgramDaycarried paired namestoo. The catalog makes that redundant and expensive - all 649 of its entries
would need both languages written and maintained by hand, and the upstream
source has only English.
Scope limit, deliberate
The app's interface stays bilingual. Buttons, labels and screens still
switch to Persian from
messages/, Persian is still the default locale, and RTLlayout is untouched. Only content collapses.
Columns are dropped rather than merged - there is no data to preserve.
The two things worth more than the column change
1. The name resolver collapses to a single
findUnique.It used to be a two-step dance:
nameFaby unique key, thennameEnwith anexplicit
orderBy: { id: "asc" }- becausenameEnwas not unique, and withoutthat tie-break a program could silently rebind to a different exercise between
uploads (#45). One name under
@@unique([userId, name])removes the ambiguitythat bug lived in, so the tie-break has nothing left to protect.
2.
AmbiguousExerciseErroris deleted outright.It existed because a non-unique
nameEncould match several rows and the strictresolver had to refuse rather than quietly report the wrong lift's numbers. A
name now matches at most one row, so the situation cannot arise. The tests that
described it are rewritten to assert the constraint that replaced it.
InfoPanelalso loses itslocaleprop, which existed only to pick a languageper field.
Verified in the running app
Not only in tests. Uploaded the real
strength_upper_v9.yaml, then checked:لاگ وزنه,پیشرفت,پروفایل,یادداشت برای مربی)dir="rtl"on the Persian page,dir="ltr"on the English oneundefined,NaNor[object Object]reaches the rendered markupThat last check matters: a mechanical rename across 25 files is exactly the kind
of change that types accept and screens quietly break on.
Tests
363 passing, 0 failing.
tsc --noEmitandeslintclean.Obsolete two-name tests are rewritten rather than deleted - each now asserts the
property that survived (per-account name scoping, binding to an existing row
instead of minting a duplicate, the unique constraint refusing a second row).