refactor: field type switching #197
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideRefactors database field-type switching to be lazy and source-aware, adding a Class diagram for lazy field type switching and decodingclassDiagram
class YDatabaseCell {
+get(key)
+set(key, value)
}
class YDatabaseField {
+get(key)
+set(key, value)
}
class User
class FieldType {
<<enumeration>>
RichText
Number
Checkbox
SingleSelect
MultiSelect
Checklist
DateTime
Relation
FileMedia
Person
Time
}
class YjsDatabaseKey {
<<enumeration>>
field_type
source_field_type
data
type
}
class Cell {
+fieldType FieldType
+data unknown
}
class DateTimeCell {
+fieldType FieldType
+data unknown
+endTimestamp number
+includeTime boolean
+isRange boolean
+reminderId string
}
class ChecklistCellData {
+options SelectOption[]
+selectedOptionIds string[]
+percentage number
}
class SelectOption {
+id string
+name string
+color number
}
class CellParser {
+parseYDatabaseCommonCellToCell(cell YDatabaseCell) Cell
+parseYDatabaseCellToCell(cell YDatabaseCell, field YDatabaseField) Cell
+parseYDatabaseDateTimeCellToCell(cell YDatabaseCell) DateTimeCell
+transformCellData(cell YDatabaseCell, sourceType FieldType, targetType FieldType, field YDatabaseField) unknown
+stringifyFromSource(cell YDatabaseCell, field YDatabaseField, sourceType FieldType, currentUser User) string
+stringifyChecklistStruct(checklist ChecklistCellData) string
}
class ChecklistUtils {
+parseChecklistData(data string) ChecklistCellData
+parseChecklistFlexible(data string) ChecklistCellData
+stringifyChecklist(options SelectOption[], selected string[]) string
+parseChecklistTextToStruct(text string) ChecklistCellData
}
class TextUtils {
+parseCheckboxValue(data) boolean
+parseTimeStringToMs(text string) string
}
class DecodeUtils {
+decodeCellToText(cell YDatabaseCell, field YDatabaseField, currentUser User) string
+decodeCellForSort(cell YDatabaseCell, field YDatabaseField, currentUser User) boolean string number
}
class SortUtils {
+sortBy(rows, sorts, fields) Row[]
+defaultValueForSort(fieldType FieldType, condition number) boolean string number
}
class FilterUtils {
+filterBy(rows, filters, fields, rowMetas) Row[]
+checkboxFilterCheck(data, condition number) boolean
+checklistFilterCheck(data, content string, condition number) boolean
+selectOptionFilterCheck(field YDatabaseField, data, content string, condition number) boolean
}
class GroupUtils {
+groupByCheckbox(rows, fieldId string, rowMetas) Map
+groupBySelectOption(rows, fieldId string, rowMetas, field YDatabaseField) Map
}
CellParser --> YDatabaseCell : reads
CellParser --> YDatabaseField : reads
CellParser --> FieldType : uses
CellParser --> YjsDatabaseKey : uses
CellParser --> ChecklistUtils : uses
CellParser --> TextUtils : uses
CellParser --> DateTimeCell : creates
CellParser --> Cell : creates
DecodeUtils --> YDatabaseCell : reads
DecodeUtils --> YDatabaseField : reads
DecodeUtils --> FieldType : uses
DecodeUtils --> ChecklistUtils : uses
DecodeUtils --> TextUtils : uses
SortUtils --> DecodeUtils : uses
SortUtils --> FieldType : uses
FilterUtils --> DecodeUtils : uses decodeCellToText
FilterUtils --> TextUtils : uses parseCheckboxValue
FilterUtils --> ChecklistUtils : uses parseChecklistFlexible
GroupUtils --> TextUtils : uses parseCheckboxValue
GroupUtils --> ChecklistUtils : uses parseChecklistFlexible
ChecklistUtils --> SelectOption : creates
ChecklistUtils --> ChecklistCellData : creates
Flow diagram for lazy field type switching and on-demand conversionflowchart TD
A[User changes field type in UI] --> B[useSwitchPropertyType hook]
B --> C[Iterate all rows in database]
C --> D[For each row, get cells map]
D --> E[Get cell for switched fieldId]
E --> F{Cell exists?}
F -- No --> C
F -- Yes --> G[Read oldCellType from field_type]
G --> H[Read current data from data]
H --> I[Write oldCellType to source_field_type]
I --> J[Clone data if Y.Array, else keep as is]
J --> K[Update cell.field_type to new FieldType]
K --> L[Persist cloned original data into data]
L --> M[Update last_modified on cell and row]
M --> C
subgraph RenderAndInteraction
N[Selector hooks useCellSelector] --> O[parseYDatabaseCellToCell with cell and field]
O --> P[Read source_field_type and field_type]
P --> Q{sourceType equals targetType?}
Q -- No --> R[transformCellData with cell, sourceType, targetType, field]
R --> S[Return converted Cell value]
Q -- Yes --> S[Return parsed Cell value]
T[Sorting: sortBy] --> U[decodeCellForSort with cell and field]
U --> V[Use FieldType-specific logic and source_field_type]
W[Filtering: filterBy] --> X[decodeCellToText with cell and field]
X --> Y[Apply type-specific filter checks]
Z[Grouping: groupByCheckbox / groupBySelectOption] --> AA[Read source_field_type and data]
AA --> AB[Map Checklist, Checkbox, Select values appropriately]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
🥷 Ninja i18n – 🛎️ Translations need to be updatedProject
|
| lint rule | new reports | level | link |
|---|---|---|---|
| Missing translation | 28 | warning | contribute (via Fink 🐦) |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- There is now overlapping conversion logic between
getCellDataText/stringifyFromSourceand the newdecodeCellToText/decodeCellForSorthelpers; consider centralizing the type-conversion rules in a single module to avoid future drift between display, filtering, and sorting behavior. - Changing
generateUUIDfrom a v5 (time-seeded) UUID to a random v4 UUID alters the determinism characteristics of generated IDs; double-check that any callers (e.g., checklist option regeneration or diffing logic) do not rely on predictable/stable IDs across sessions. - Several of the new Jest tests assert tautologies like
expect(result.length >= 0).toBe(true)or accept multiple very loose outcomes; tightening these expectations to the actual intended behavior would make the tests a more reliable safety net for future refactors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There is now overlapping conversion logic between `getCellDataText`/`stringifyFromSource` and the new `decodeCellToText`/`decodeCellForSort` helpers; consider centralizing the type-conversion rules in a single module to avoid future drift between display, filtering, and sorting behavior.
- Changing `generateUUID` from a v5 (time-seeded) UUID to a random v4 UUID alters the determinism characteristics of generated IDs; double-check that any callers (e.g., checklist option regeneration or diffing logic) do not rely on predictable/stable IDs across sessions.
- Several of the new Jest tests assert tautologies like `expect(result.length >= 0).toBe(true)` or accept multiple very loose outcomes; tightening these expectations to the actual intended behavior would make the tests a more reliable safety net for future refactors.
## Individual Comments
### Comment 1
<location> `src/application/database-yjs/decode.ts:21-22` </location>
<code_context>
+ const sourceType = Number(cell.get(YjsDatabaseKey.source_field_type) ?? cell.get(YjsDatabaseKey.field_type)) as FieldType;
+ const data = cell.get(YjsDatabaseKey.data);
+
+ // If types match and data is string/number, return raw string.
+ if (sourceType === targetType && (typeof data === 'string' || typeof data === 'number')) {
+ return String(data);
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** The early return for matching source/target types bypasses the specialized per-type decoding logic.
This short‑circuit causes DateTime, URL, Time, Checklist, and Select cells to skip their specialized decoding when `sourceType === targetType` and `data` is a string/number (e.g. DateTime won’t use `getDateCellStr`, URL won’t be trimmed, Time won’t be parsed). As a result, `decodeCellToText` won’t apply the new logic for these types when filtering. Consider removing this early return or limiting it to plain text types only, so other field types always pass through the switch‑based decoding.
</issue_to_address>
### Comment 2
<location> `src/application/database-yjs/decode.ts:103-105` </location>
<code_context>
+ return '';
+ }
+
+ case FieldType.Number:
+ if (typeof data === 'string' || typeof data === 'number') {
+ return String(data);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** decodeCellForSort returns mixed types (numbers and empty strings) for numeric-like fields, which can lead to inconsistent sorting.
For `FieldType.Number` and `FieldType.DateTime`, failures return `''` while successes return numbers, so the sort key can mix `number`, `string`, and `boolean` (for Checkbox). Depending on the comparator, this number/string mix can cause unpredictable ordering or coercion. Consider using a numeric sentinel (e.g. `Infinity`, `-Infinity`, or `NaN`) for unparsable numeric/date values so those sort keys stay consistently numeric.
Suggested implementation:
```typescript
case FieldType.Number: {
// Always return a numeric sort key for numeric fields; use NaN as a sentinel for invalid/unparsable values
if (typeof data === 'number') {
return data;
}
if (typeof data === 'string') {
const parsed = Number(data);
return Number.isNaN(parsed) ? NaN : parsed;
}
return NaN;
}
```
```typescript
case FieldType.DateTime: {
// Always return a numeric sort key (timestamp in ms); use NaN for invalid dates
if (typeof data === 'string' || typeof data === 'number' || data instanceof Date) {
const date = data instanceof Date ? data : new Date(data);
const time = date.getTime();
return Number.isNaN(time) ? NaN : time;
}
return NaN;
}
```
1. If the existing `FieldType.DateTime` implementation differs from the `SEARCH` block above, adapt the `SEARCH` section to match the current code around the `DateTime` case and apply the same replacement logic.
2. Ensure that the comparator used with `decodeCellForSort` handles `NaN` appropriately (e.g. by normalizing `NaN` to sort first/last), since `NaN` is not directly comparable with `===` and standard numeric comparisons.
3. If `decodeCellForSort` is used in any non-sorting context that expects strings for numeric/date fields, verify that those call sites are updated (or use a different decode function) so they do not rely on the previous string-returning behavior.
</issue_to_address>
### Comment 3
<location> `src/application/database-yjs/fields/text/utils.ts:38-41` </location>
<code_context>
+ return false;
+}
+
+export function parseTimeStringToMs(text: string): string | null {
+ const trimmed = text.trim();
+
+ if (!trimmed) return '';
+
+ if (!Number.isNaN(Number(trimmed))) {
</code_context>
<issue_to_address>
**nitpick (bug_risk):** parseTimeStringToMs’s return type doesn’t match its semantics for empty input.
The function is typed as `string | null`, where `null` indicates an unparseable value, but an empty string currently returns `''`. This makes the distinction between `''` and `null` subtle and error-prone. Please either return `null` for empty input to match the existing semantics, or update the return type and docs to explicitly include `''` as a valid “no time” value.
</issue_to_address>
### Comment 4
<location> `src/application/database-yjs/cell.parse.ts:66` </location>
<code_context>
return value;
}
+function transformCellData(
+ cell: YDatabaseCell,
+ sourceType: FieldType,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared helpers for checkbox, select, checklist, and time conversions to deduplicate logic across `transformCellData`, `getCellDataText`, and `stringifyFromSource` and reduce their coupling.
You’ve added a lot of valuable conversion paths, but `transformCellData`, `getCellDataText`, and `stringifyFromSource` are now tightly coupled and duplicating logic. You can keep all functionality while reducing complexity by extracting a few small, shared helpers and reusing them across these switches.
### 1. Extract reusable checkbox helpers
You stringify checkbox values in at least three places (`transformCellData`, `getCellDataText`, `stringifyFromSource`):
```ts
if (typeof data === 'string' || typeof data === 'number' || typeof data === 'boolean') {
const isChecked = parseCheckboxValue(data);
return isChecked ? 'Yes' : 'No';
}
```
Create a tiny helper:
```ts
function stringifyCheckboxData(data: unknown): string {
if (typeof data === 'string' || typeof data === 'number' || typeof data === 'boolean') {
return parseCheckboxValue(data) ? 'Yes' : 'No';
}
return '';
}
```
Then use it in all three locations:
```ts
// transformCellData → Checkbox from RichText
case FieldType.Checkbox:
if (sourceType === FieldType.RichText) {
return stringifyCheckboxData(data);
}
// ...
// getCellDataText → Checkbox display
case FieldType.Checkbox: {
const data = cell.get(YjsDatabaseKey.data);
return stringifyCheckboxData(data);
}
// stringifyFromSource → Checkbox as RichText
case FieldType.Checkbox:
return stringifyCheckboxData(data);
```
This trims duplicate branches and makes future checkbox changes safe and centralized.
### 2. Extract select ID ↔ name mapping helper
You repeat the “select ids to names” logic with slight variations:
```ts
data
.split(',')
.map((item) => {
const option = options.find((option) => option?.id === item || option?.name === item);
return option?.name || '';
})
.filter(Boolean)
.join(',');
```
and similarly in `stringifyFromSource`.
Extract one helper and use it in both `getCellDataText` and `stringifyFromSource`:
```ts
function selectIdsToNamesCSV(data: unknown, options: SelectOption[]): string {
if (typeof data !== 'string') return '';
return data
.split(',')
.map((idOrName) => options.find((o) => o.id === idOrName || o.name === idOrName)?.name)
.filter(Boolean)
.join(',');
}
```
Usage:
```ts
// getCellDataText
case FieldType.SingleSelect:
case FieldType.MultiSelect: {
const data = cell.get(YjsDatabaseKey.data);
const options = parseSelectOptionTypeOptions(field)?.options || [];
// checklist-origin special-case stays
if (typeof data === 'string') {
const checklist = parseChecklistFlexible(data);
if (checklist && sourceType === FieldType.Checklist) {
const selectedNames = checklist.selectedOptionIds
?.map((id) => options.find((opt) => opt.id === id || opt.name === id)?.name)
.filter(Boolean);
if (selectedNames?.length) return selectedNames.join(',');
}
}
return selectIdsToNamesCSV(data, options);
}
// stringifyFromSource
case FieldType.SingleSelect:
case FieldType.MultiSelect: {
if (!field) return '';
const options = parseSelectOptionTypeOptions(field)?.options || [];
return selectIdsToNamesCSV(data, options);
}
```
This removes duplication and keeps the checklist special case intact.
### 3. Extract checklist stringify helper (struct vs options + ids)
You now have both:
- `stringifyChecklistStruct(checklist: ChecklistCellData)` (new),
- `stringifyChecklist(options, selectedOptionIds)` (existing).
You can avoid the extra mental overhead by wrapping the existing function instead of re-implementing the shape in multiple places:
```ts
function stringifyChecklistFromStruct(checklist: ChecklistCellData): string {
return stringifyChecklist(checklist.options || [], checklist.selectedOptionIds || []);
}
```
Then:
```ts
// transformCellData – RichText → Checklist
if (sourceType === FieldType.RichText) {
const parsed = parseChecklistFlexible(data);
if (!parsed) return '';
// if you truly need the struct JSON, keep stringifyChecklistStruct;
// otherwise use the new wrapper:
return stringifyChecklistFromStruct(parsed);
}
// getCellDataText – Checklist
case FieldType.Checklist: {
const cellData = cell.get(YjsDatabaseKey.data);
if (typeof cellData === 'string') {
const parsed = parseChecklistFlexible(cellData);
if (!parsed) return '';
return stringifyChecklistFromStruct(parsed);
}
return '';
}
// stringifyFromSource – Checklist
case FieldType.Checklist: {
if (typeof data === 'string') {
const parsed = parseChecklistFlexible(data);
if (!parsed) return '';
return stringifyChecklistFromStruct(parsed);
}
return '';
}
```
If you do need both shapes (struct JSON vs rendered string), keep both helpers but ensure each goes through a single code path (`ChecklistCellData` → underlying format) instead of duplicating the object shape inline.
### 4. Extract time conversion helper
`Time` handling is also duplicated in `transformCellData`, `getCellDataText`, and `stringifyFromSource`:
```ts
if (typeof data === 'string') return parseTimeStringToMs(data) ?? '';
if (typeof data === 'number') return String(data);
```
Create a shared helper:
```ts
function normalizeTimeDataToMsString(data: unknown): string {
if (typeof data === 'number') return String(data);
if (typeof data === 'string') {
const parsed = parseTimeStringToMs(data);
return parsed ?? '';
}
return '';
}
```
Then:
```ts
// transformCellData → Time
case FieldType.Time:
return normalizeTimeDataToMsString(data);
// getCellDataText → Time
case FieldType.Time: {
const data = cell.get(YjsDatabaseKey.data);
if (data === undefined || data === null) return '';
return normalizeTimeDataToMsString(data);
}
// stringifyFromSource → Time
case FieldType.Time:
return normalizeTimeDataToMsString(data) || (typeof data === 'string' ? data : '');
```
This keeps all current behaviour but collapses 3 similar branches into a single well‑named abstraction.
---
By pulling out these small helpers and calling them from `transformCellData`, `getCellDataText`, and `stringifyFromSource`, you keep the new cross‑type conversion features but reduce the “god switch” effect and duplication.
</issue_to_address>
### Comment 5
<location> `src/application/database-yjs/filter.ts:358` </location>
<code_context>
}
-export function selectOptionFilterCheck(data: string, content: string, condition: number) {
+export function selectOptionFilterCheck(field: YDatabaseField, data: unknown, content: string, condition: number) {
+ const filterOptionIds = content.split(',').filter((item) => item.trim() !== '');
+ const typeOption = parseSelectOptionTypeOptions(field);
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the logic that converts a cell’s raw value into normalized select option IDs into a reusable helper so `selectOptionFilterCheck` only handles filter conditions.
You can simplify and de‑duplicate the added logic by extracting the “cell → selected option IDs” behavior into a small helper and reusing it inside `selectOptionFilterCheck` (and later in grouping/decoding).
Right now `selectOptionFilterCheck`:
- Parses checklist vs comma‑separated string.
- Resolves checklist option IDs/names to names.
- Resolves those names/IDs again against `typeOption.options`.
- Then re‑normalizes them a second time with `selectedIdsByName`.
You can move this into a single helper that returns a clean `string[]` of normalized option IDs and remove the double mapping:
```ts
function getSelectedOptionIdsFromCell(field: YDatabaseField, data: unknown): string[] {
if (typeof data !== 'string' || !data.trim()) return [];
const typeOption = parseSelectOptionTypeOptions(field);
const options = typeOption?.options || [];
const checklist = parseChecklistFlexible(data);
if (checklist) {
const checkedNamesOrIds =
checklist.selectedOptionIds
?.map((idOrName) => {
const fromChecklist = checklist.options?.find((opt) => opt.id === idOrName)?.name;
return fromChecklist ?? idOrName;
})
.filter(Boolean) ?? [];
return checkedNamesOrIds
.map((idOrName) => options.find((opt) => opt.id === idOrName || opt.name === idOrName)?.id)
.filter((item): item is string => Boolean(item));
}
// non-checklist encoding: assume comma-separated IDs or names
const raw = data
.split(',')
.map((item) => item.trim())
.filter(Boolean);
return raw
.map((idOrName) => options.find((opt) => opt.id === idOrName || opt.name === idOrName)?.id)
.filter((item): item is string => Boolean(item));
}
```
`selectOptionFilterCheck` can then be reduced to pure filtering logic:
```ts
export function selectOptionFilterCheck(
field: YDatabaseField,
data: unknown,
content: string,
condition: number,
) {
const selectedOptionIds = getSelectedOptionIdsFromCell(field, data);
const filterOptionIds = content
.split(',')
.map((item) => item.trim())
.filter(Boolean);
if (SelectOptionFilterCondition.OptionIsEmpty === condition) {
return selectedOptionIds.length === 0;
}
if (SelectOptionFilterCondition.OptionIsNotEmpty === condition) {
return selectedOptionIds.length > 0;
}
switch (condition) {
case SelectOptionFilterCondition.OptionIs:
case SelectOptionFilterCondition.OptionContains:
if (!content) return true;
return some(filterOptionIds, (option) => selectedOptionIds.includes(option));
case SelectOptionFilterCondition.OptionIsNot:
case SelectOptionFilterCondition.OptionDoesNotContain:
if (!content) return true;
return every(filterOptionIds, (option) => !selectedOptionIds.includes(option));
default:
return false;
}
}
```
This keeps all current behavior (including checklist support and name/ID resolution), but:
- Removes duplicated “id or name → id” mapping.
- Keeps `selectOptionFilterCheck` focused on filter conditions only.
- Gives you a reusable `getSelectedOptionIdsFromCell` that can be shared with grouping and other decoding logic later.
</issue_to_address>
### Comment 6
<location> `src/application/database-yjs/group.ts:87` </location>
<code_context>
- const cellData = getCellData(row.id, fieldId, rowMetas);
-
- const checked = getChecked(cellData as string);
+ const cell = getCell(row.id, fieldId, rowMetas);
+ const cellData = cell?.get(YjsDatabaseKey.data);
+ const checked = parseCheckboxValue(cellData as string);
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the new select/checklist and checkbox decoding logic into shared normalization helpers so grouping and filtering functions only handle already-normalized values.
You can reduce the added complexity by extracting the cross‑type selection/checkbox decoding into shared helpers, then using them both here and in filters.
### 1. Normalize select/checklist option IDs in a shared helper
Create a helper (e.g. in a shared `select`/`options` utils module) that encapsulates `source_field_type`, checklist parsing, and ID/name reconciliation:
```ts
// selectOptionNormalization.ts
import { FieldType } from '@/application/database-yjs/database.type';
import { parseChecklistFlexible } from '@/application/database-yjs/fields/checklist/parse';
import { YDoc, YjsDatabaseKey, YDatabaseField } from '@/application/types';
import { getCell } from '@/application/database-yjs/const';
import { parseSelectOptionTypeOptions } from '@/application/database-yjs/fields';
export function getSelectedOptionIdsFromRow(
rowId: string,
field: YDatabaseField,
rowMetas: Record<string, YDoc>
): string[] {
const fieldId = field.get(YjsDatabaseKey.id) as string;
const cell = getCell(rowId, fieldId, rowMetas);
if (!cell) return [];
const cellData = cell.get(YjsDatabaseKey.data);
const sourceType = Number(
cell.get(YjsDatabaseKey.source_field_type) ?? cell.get(YjsDatabaseKey.field_type)
) as FieldType;
const typeOption = parseSelectOptionTypeOptions(field);
if (!typeOption) return [];
const resolveId = (idOrName: string) =>
typeOption.options.find((opt) => opt.id === idOrName || opt.name === idOrName)?.id;
if (sourceType === FieldType.Checklist && typeof cellData === 'string') {
const checklist = parseChecklistFlexible(cellData);
return (
checklist?.selectedOptionIds
?.map(resolveId)
.filter((id): id is string => Boolean(id)) ?? []
);
}
if (typeof cellData === 'string') {
return (
cellData
.split(',')
.map((v) => v.trim())
.map(resolveId)
.filter((id): id is string => Boolean(id)) ?? []
);
}
return [];
}
```
Then `groupBySelectOption` can focus only on grouping logic:
```ts
// in grouping file
import { getSelectedOptionIdsFromRow } from '@/application/database-yjs/fields/selectOptionNormalization';
rows.forEach((row) => {
if (!rowMetas[row.id]) {
return;
}
const selectedIds = getSelectedOptionIdsFromRow(row.id, field, rowMetas);
if (selectedIds.length === 0) {
if (!result.has(fieldId)) {
return;
}
const group = result.get(fieldId) ?? [];
group.push(row);
result.set(fieldId, group);
return;
}
selectedIds.forEach((id) => {
if (!result.has(id)) {
return;
}
const group = result.get(id) ?? [];
group.push(row);
result.set(id, group);
});
});
```
You can similarly replace any duplicated logic in `selectOptionFilterCheck` to call `getSelectedOptionIdsFromRow` or a variant that works directly from a `cell`/`rawData`, keeping the ID/name reconciliation logic in one place.
### 2. Extract checkbox decoding from grouping
`groupByCheckbox` now knows about `getCell` and `parseCheckboxValue`. You can hide that behind a small helper that is reusable in filters or elsewhere:
```ts
// checkboxNormalization.ts
import { getCell } from '@/application/database-yjs/const';
import { YDoc, YjsDatabaseKey, YDatabaseField } from '@/application/types';
import { parseCheckboxValue } from '@/application/database-yjs/fields/text/utils';
export function getCheckboxValueFromRow(
rowId: string,
field: YDatabaseField,
rowMetas: Record<string, YDoc>
): boolean {
const fieldId = field.get(YjsDatabaseKey.id) as string;
const cell = getCell(rowId, fieldId, rowMetas);
const cellData = cell?.get(YjsDatabaseKey.data);
return parseCheckboxValue(cellData as string);
}
```
Then simplify `groupByCheckbox`:
```ts
// in grouping file
import { getCheckboxValueFromRow } from '@/application/database-yjs/fields/checkboxNormalization';
rows.forEach((row) => {
if (!rowMetas[row.id]) {
return;
}
const checked = getCheckboxValueFromRow(row.id, field, rowMetas);
const groupName = checked ? 'Yes' : 'No';
if (!result.has(groupName)) {
return;
}
const group = result.get(groupName) ?? [];
group.push(row);
result.set(groupName, group);
});
```
This keeps all existing behavior (including checklist and ID/name handling) but centralizes the complex normalization. The grouping and filtering modules then only handle grouping/filtering logic on already-normalized IDs/booleans, which lowers cognitive load and avoids future divergence.
</issue_to_address>
### Comment 7
<location> `src/application/database-yjs/decode.ts:12` </location>
<code_context>
+ * Decode a cell to a string representation for rendering/filtering/sorting,
+ * using the cell's recorded source type when it differs from the field's current type.
+ */
+export function decodeCellToText(
+ cell: YDatabaseCell,
+ field: YDatabaseField,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared helpers for select/checklist name resolution and delegating more sorting logic to `decodeCellToText` to simplify both switch statements and avoid duplication.
You can reduce duplication and make this easier to follow by extracting small helpers and reusing `decodeCellToText` more aggressively inside `decodeCellForSort`.
### 1. Deduplicate checklist → names and select ID/name resolution
Both functions do:
- `parseChecklistFlexible(data)`
- `parseSelectOptionTypeOptions(field)`
- `options.find((opt) => opt.id === id || opt.name === id)?.name`
- `join(',')`
You can centralize that into a couple of primitives:
```ts
function getSelectOptions(field: YDatabaseField) {
return parseSelectOptionTypeOptions(field)?.options || [];
}
function resolveSelectNamesFromIdsOrNames(
field: YDatabaseField,
idsOrNames: string[]
): string[] {
const options = getSelectOptions(field);
return idsOrNames
.map((idOrName) => options.find((opt) => opt.id === idOrName || opt.name === idOrName)?.name)
.filter((name): name is string => Boolean(name));
}
function getChecklistSelectedNamesFromSelectField(
field: YDatabaseField,
data: string
): string[] {
const parsed = parseChecklistFlexible(data);
if (!parsed?.selectedOptionIds?.length) return [];
return resolveSelectNamesFromIdsOrNames(field, parsed.selectedOptionIds);
}
```
Then `decodeCellToText` select branches become much simpler and reuse the same logic:
```ts
case FieldType.Checklist: {
if (typeof data !== 'string') return '';
const parsed = parseChecklistFlexible(data);
if (!parsed) return '';
return stringifyChecklist(parsed.options || [], parsed.selectedOptionIds || []);
}
case FieldType.SingleSelect:
case FieldType.MultiSelect: {
if (typeof data !== 'string') return '';
if (sourceType === FieldType.Checklist) {
const names = getChecklistSelectedNamesFromSelectField(field, data);
if (names.length) return names.join(',');
}
const idsOrNames = data.split(',');
const names = resolveSelectNamesFromIdsOrNames(field, idsOrNames);
return names.join(',');
}
```
`decodeCellForSort` can then reuse the same primitives instead of re‑implementing the mapping:
```ts
case FieldType.SingleSelect:
case FieldType.MultiSelect: {
if (typeof data !== 'string') return '';
if (sourceType === FieldType.Checklist) {
return getChecklistSelectedNamesFromSelectField(field, data).join(',');
}
const idsOrNames = data.split(',');
return resolveSelectNamesFromIdsOrNames(field, idsOrNames).join(',');
}
```
### 2. Make `decodeCellForSort` leaner by delegating to `decodeCellToText`
You already fall back to `decodeCellToText` in `default`. You can extend that pattern to all text‑sortable types, keeping only genuinely non‑string sort keys in the switch:
```ts
export function decodeCellForSort(
cell: YDatabaseCell,
field: YDatabaseField,
currentUser?: User
): string | number | boolean {
const targetType = Number(field.get(YjsDatabaseKey.type)) as FieldType;
const data = cell.get(YjsDatabaseKey.data);
switch (targetType) {
case FieldType.Checkbox:
if (typeof data === 'string' || typeof data === 'number' || typeof data === 'boolean') {
return parseCheckboxValue(data);
}
return false;
case FieldType.Number:
case FieldType.DateTime:
if (typeof data === 'number') return data;
if (typeof data === 'string' && data.trim() !== '' && !Number.isNaN(Number(data))) {
return Number(data);
}
return '';
case FieldType.Checklist: {
if (typeof data !== 'string') return 0;
const parsed = parseChecklistFlexible(data);
return parsed?.percentage ?? 0;
}
default:
// All other types: use text representation as sort key
return decodeCellToText(cell, field, currentUser);
}
}
```
If you still need specialized behavior for selects, you can either:
- Keep a small select case that *delegates* to the helper (`resolveSelectNamesFromIdsOrNames`), or
- Rely on `decodeCellToText` for those and treat them as plain text sort keys.
These two refactors should reduce the size of both switches, eliminate repeated logic for select/checklist handling, and make this module the canonical place for ID→name/string conversions without changing behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+21
to
+22
| // If types match and data is string/number, return raw string. | ||
| if (sourceType === targetType && (typeof data === 'string' || typeof data === 'number')) { |
There was a problem hiding this comment.
issue (bug_risk): The early return for matching source/target types bypasses the specialized per-type decoding logic.
This short‑circuit causes DateTime, URL, Time, Checklist, and Select cells to skip their specialized decoding when sourceType === targetType and data is a string/number (e.g. DateTime won’t use getDateCellStr, URL won’t be trimmed, Time won’t be parsed). As a result, decodeCellToText won’t apply the new logic for these types when filtering. Consider removing this early return or limiting it to plain text types only, so other field types always pass through the switch‑based decoding.
Comment on lines
+103
to
+105
| case FieldType.Number: | ||
| if (typeof data === 'number') return data; | ||
| if (typeof data === 'string' && data.trim() !== '' && !Number.isNaN(Number(data))) { |
There was a problem hiding this comment.
suggestion (bug_risk): decodeCellForSort returns mixed types (numbers and empty strings) for numeric-like fields, which can lead to inconsistent sorting.
For FieldType.Number and FieldType.DateTime, failures return '' while successes return numbers, so the sort key can mix number, string, and boolean (for Checkbox). Depending on the comparator, this number/string mix can cause unpredictable ordering or coercion. Consider using a numeric sentinel (e.g. Infinity, -Infinity, or NaN) for unparsable numeric/date values so those sort keys stay consistently numeric.
Suggested implementation:
case FieldType.Number: {
// Always return a numeric sort key for numeric fields; use NaN as a sentinel for invalid/unparsable values
if (typeof data === 'number') {
return data;
}
if (typeof data === 'string') {
const parsed = Number(data);
return Number.isNaN(parsed) ? NaN : parsed;
}
return NaN;
} case FieldType.DateTime: {
// Always return a numeric sort key (timestamp in ms); use NaN for invalid dates
if (typeof data === 'string' || typeof data === 'number' || data instanceof Date) {
const date = data instanceof Date ? data : new Date(data);
const time = date.getTime();
return Number.isNaN(time) ? NaN : time;
}
return NaN;
}- If the existing
FieldType.DateTimeimplementation differs from theSEARCHblock above, adapt theSEARCHsection to match the current code around theDateTimecase and apply the same replacement logic. - Ensure that the comparator used with
decodeCellForSorthandlesNaNappropriately (e.g. by normalizingNaNto sort first/last), sinceNaNis not directly comparable with===and standard numeric comparisons. - If
decodeCellForSortis used in any non-sorting context that expects strings for numeric/date fields, verify that those call sites are updated (or use a different decode function) so they do not rely on the previous string-returning behavior.
Comment on lines
+38
to
+41
| export function parseTimeStringToMs(text: string): string | null { | ||
| const trimmed = text.trim(); | ||
|
|
||
| if (!trimmed) return ''; |
There was a problem hiding this comment.
nitpick (bug_risk): parseTimeStringToMs’s return type doesn’t match its semantics for empty input.
The function is typed as string | null, where null indicates an unparseable value, but an empty string currently returns ''. This makes the distinction between '' and null subtle and error-prone. Please either return null for empty input to match the existing semantics, or update the return type and docs to explicitly include '' as a valid “no time” value.
josue693
pushed a commit
to josue693/AppFlowy-Web
that referenced
this pull request
Dec 21, 2025
* refactor: field type switching * chore: add test * chore: use nanoid
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Record original field types on cells and lazily decode/transform data when switching database field types, adding support for time fields and checklist/select/checkbox conversions while updating sorting, filtering, grouping, and selectors to use the new decoding logic.
New Features:
Bug Fixes:
Enhancements:
Tests: