Subtitle add#223
Conversation
- Introduced a new sub-title field in GroupMetadataDTO and GroupMetadataInput interfaces. - Updated GroupFormPage to include sub-title input in the form and validation. - Enhanced GroupDetailsPage to display the sub-title if provided. - Adjusted group schema validation to require sub-title along with title and description.
- Updated `resolveSeriesImageKey` to ensure proper handling of image strings. - Refactored `resolveGroupBannerUrl` and `resolveGroupAvatarUrl` to utilize `resolveDashboardItemImageUrl` for improved URL resolution. - Simplified image URL mapping in `groupLinkedPlansToFkOptions` for better clarity and maintainability.
Confidence Score: 3/5The feature addition itself is straightforward, but making sub_title and description required in the schema will silently block all users from saving changes to existing groups that lack those fields. Editors of any existing group without a sub-title or description will hit a blocking form validation error every time they try to save — even for unrelated changes like toggling visibility. This affects the entire existing dataset and is not gated behind a migration or a backward-compatible default. src/schema/GroupSchema.ts — the .min(1) constraints on sub_title and description are the root of the breaking-change concern and should be reviewed carefully before merging. Reviews (1): Last reviewed commit: "refactor(api): enhance image URL resolut..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| title: z.string().trim().min(1, "Title is required"), | ||
| sub_title: z.string().trim().min(1, "Sub-title is required"), | ||
| description: z.string().trim().min(1, "Description is required"), | ||
| }); |
There was a problem hiding this comment.
Both sub_title and description are now enforced with .min(1), but existing groups loaded from the API can have null/undefined for either field. The hydration code defaults both to "" via meta.sub_title ?? "" and meta.description ?? "", which immediately fails the .min(1) check. Any user who tries to edit an existing group — even to toggle is_public — will be blocked by a validation error on every language block until they fill in a subtitle and a description. The old schema treated description as optional (no .min(1)), and sub_title did not exist, so this is a breaking change for all pre-existing records.
| if (!block) continue; | ||
| out.push({ | ||
| language: code, | ||
| title: block.title.trim(), |
There was a problem hiding this comment.
The previous guard was if (!block?.title?.trim()) continue, which protected the API call if a block somehow had an empty title string. The new guard only skips undefined blocks, so a block that passes through with title: "" (e.g., if schema validation is somehow bypassed, or if buildGroupMetadata is called programmatically outside the form) will be sent to the backend with an empty title.
| if (!block) continue; | |
| out.push({ | |
| language: code, | |
| title: block.title.trim(), | |
| if (!block?.title?.trim()) continue; | |
| out.push({ | |
| language: code, | |
| title: block.title.trim(), |
| <Pecha.FormItem> | ||
| <Pecha.FormLabel className="text-sm font-bold"> | ||
| {languageLabelForCode(code)} title | ||
| <span className="text-destructive"> *</span> | ||
| </Pecha.FormLabel> | ||
| <Pecha.FormControl> | ||
| <Pecha.Input | ||
| className="h-12 bg-white dark:bg-[#181818]" | ||
| {...field} | ||
| /> | ||
| </Pecha.FormControl> | ||
| <Pecha.FormMessage /> |
There was a problem hiding this comment.
The sub-title label shows a red asterisk (*) indicating a required field, and the schema enforces .min(1). However, sub-title is a brand-new optional field on the server (sub_title?: string | null in GroupMetadataDTO). Marking it required in the UI when it may not be enforced server-side (and existing records will never have it) sets a misleading expectation. If sub-title is genuinely required going forward, this should be a coordinated backend + migration change; if it is optional, the asterisk and .min(1) should be removed.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
No description provided.