Conversation
WalkthroughThis pull request implements improvements for nested object field handling across the application stack. Backend YAML parsing now supports indentation-based nested structures; schema processing distinguishes between dynamic objects (with Changes
Sequence DiagramsequenceDiagram
actor User
participant Frontend
participant Backend
participant SchemaProcessor
participant YAMLParser
User->>Frontend: Edit nested object field (e.g., metadata)
Frontend->>Frontend: Render individual nested fields grouped<br/>(no parent object input)
User->>Frontend: Submit frontmatter changes
Frontend->>Backend: Send frontmatter with nested values
Backend->>YAMLParser: Parse frontmatter with nested structure
YAMLParser->>YAMLParser: Use indentation-based scoping<br/>to build nested objects
YAMLParser-->>Backend: Return parsed nested JSON
Backend->>SchemaProcessor: Check schema for object field
alt additionalProperties: true or schema exists
SchemaProcessor-->>Backend: Treat as JSON string
else additionalProperties: false
SchemaProcessor-->>Backend: Flatten into individual fields
end
Backend->>YAMLParser: Serialize back to YAML
YAMLParser->>YAMLParser: Apply proper indentation<br/>and syntax for nested structures
YAMLParser-->>Backend: Return serialized YAML
Backend-->>Frontend: Return updated frontmatter
Frontend->>Frontend: Display updated fields with<br/>preserved structure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes The changes span multiple systems with heterogeneous modifications: backend YAML parsing logic is dense with indentation-based nesting, schema processing introduces conditional object handling, frontend filtering prevents double-rendering of parent keys, and field component props add rendering control. While individual modifications follow clear patterns, each requires separate reasoning about nested object semantics, serialization correctness, and UI state consistency. Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src-tauri/src/schema_merger.rs (1)
663-666: Do not +/-1 exclusiveMinimum/Maximum — this corrupts numeric constraints.JSON Schema exclusivity shouldn’t be converted by arithmetic (especially wrong for floats). Either preserve the exact thresholds or carry explicit exclusivity flags.
- if let Some(exclusive_min) = field_schema.exclusive_minimum { - constraints.min = Some(exclusive_min + 1.0); - } - if let Some(exclusive_max) = field_schema.exclusive_maximum { - constraints.max = Some(exclusive_max - 1.0); - } + if let Some(exclusive_min) = field_schema.exclusive_minimum { + // Preserve value; UI can indicate exclusivity separately if/when supported + constraints.min = Some(exclusive_min); + } + if let Some(exclusive_max) = field_schema.exclusive_maximum { + constraints.max = Some(exclusive_max); + }Optionally extend FieldConstraints with min_exclusive/max_exclusive for accurate UI.
🧹 Nitpick comments (4)
src/components/frontmatter/fields/FieldWrapper.tsx (2)
19-19: hideDefaultValue gating is correct; consider broader emptiness check.Looks good and matches the Boolean use case. Optional: treat empty arrays/objects as “empty” too so default text can appear when users haven’t set nested/array values.
- const isEmpty = - currentValue === undefined || currentValue === null || currentValue === '' + const isEmpty = + currentValue === undefined || + currentValue === null || + currentValue === '' || + (Array.isArray(currentValue) && currentValue.length === 0) || + (typeof currentValue === 'object' && + currentValue !== null && + !Array.isArray(currentValue) && + Object.keys(currentValue as Record<string, unknown>).length === 0)Also applies to: 31-32, 36-36, 39-41
110-117: Long/complex default values may overwhelm metadata.Optional: truncate/ellipsize object/array defaults (and add a title tooltip) to avoid noisy UI.
src-tauri/src/commands/files.rs (1)
425-458: Nested YAML parsing works; note limitations for arrays and typing.The object parser respects indentation and roundtrips well. Two optional improvements:
- parse_yaml_array currently coerces all items to strings; consider parsing booleans/numbers similarly to scalar parsing and supporting object items.
- When array/object string items contain “:” or special chars, consider quoting for YAML safety.
- let cleaned = item_value.trim_matches('"').trim_matches('\''); - array.push(Value::String(cleaned.to_string())); + let cleaned = item_value.trim_matches('"').trim_matches('\''); + if cleaned == "true" { + array.push(Value::Bool(true)); + } else if cleaned == "false" { + array.push(Value::Bool(false)); + } else if let Ok(n) = cleaned.parse::<i64>() { + array.push(Value::Number(serde_json::Number::from(n))); + } else if let Ok(n) = cleaned.parse::<f64>() { + array.push(Value::Number(serde_json::Number::from_f64(n).unwrap_or_else(|| serde_json::Number::from(0)))); + } else { + array.push(Value::String(cleaned.to_string())); + }Also applies to: 521-623
docs/tasks-todo/task-x-better-object-handling.md (1)
3-3: Minor: Consider formatting the GitHub issue URL.The bare URL could be formatted as a proper Markdown link for better readability and to satisfy the markdown linter.
Apply this diff:
-https://github.com/dannysmith/astro-editor/issues/36 +[Issue #36](https://github.com/dannysmith/astro-editor/issues/36)or wrap it in angle brackets:
-https://github.com/dannysmith/astro-editor/issues/36 +<https://github.com/dannysmith/astro-editor/issues/36>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
docs/tasks-todo/task-x-better-object-handling.md(1 hunks)src-tauri/src/commands/files.rs(6 hunks)src-tauri/src/schema_merger.rs(4 hunks)src/components/frontmatter/FrontmatterPanel.tsx(1 hunks)src/components/frontmatter/fields/BooleanField.tsx(1 hunks)src/components/frontmatter/fields/FieldWrapper.test.tsx(1 hunks)src/components/frontmatter/fields/FieldWrapper.tsx(2 hunks)test/dummy-astro-project/.astro/collections/articles.schema.json(1 hunks)test/dummy-astro-project/.astro/collections/notes.schema.json(1 hunks)test/dummy-astro-project/.astro/settings.json(1 hunks)test/dummy-astro-project/src/content.config.ts(1 hunks)test/dummy-astro-project/src/content/notes/2024-03-15-object-field-test.md(1 hunks)test/dummy-astro-project/src/content/notes/copyedit-test.md(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/components/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Direct Store Pattern in React components (access Zustand stores directly) and never use React Hook Form
Files:
src/components/frontmatter/fields/FieldWrapper.test.tsxsrc/components/frontmatter/fields/BooleanField.tsxsrc/components/frontmatter/FrontmatterPanel.tsxsrc/components/frontmatter/fields/FieldWrapper.tsx
src/components/**/*.test.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Write component tests for user interactions under src/components as .test.tsx files
Files:
src/components/frontmatter/fields/FieldWrapper.test.tsx
src-tauri/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Write and maintain Rust tests and code for the Tauri backend with modern Rust formatting (use format("{variable}"))
Files:
src-tauri/src/commands/files.rssrc-tauri/src/schema_merger.rs
🧠 Learnings (1)
📚 Learning: 2025-10-20T20:14:57.077Z
Learnt from: CR
PR: dannysmith/astro-editor#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T20:14:57.077Z
Learning: Applies to src/lib/schema.ts : Centralize Zod schema parsing for frontmatter in src/lib/schema.ts
Applied to files:
src/components/frontmatter/FrontmatterPanel.tsx
🧬 Code graph analysis (1)
src/components/frontmatter/fields/FieldWrapper.test.tsx (1)
src/components/frontmatter/fields/FieldWrapper.tsx (1)
FieldWrapper(22-62)
🪛 markdownlint-cli2 (0.18.1)
docs/tasks-todo/task-x-better-object-handling.md
3-3: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (14)
src/components/frontmatter/fields/BooleanField.tsx (1)
56-57: Good call hiding default for booleans.Prevents “Default: …” noise while getBooleanValue still respects schema defaults.
src-tauri/src/schema_merger.rs (3)
361-364: Using simple child labels aligns with fieldset grouping.LGTM; this lets the UI show parent context via grouping and keeps child labels clean.
506-522: Object handling: correct distinction between dynamic records and strict nested objects.Treating additionalProperties (true/schema) as JSON string and flattening strict objects fixes the reported bug and preserves property order via IndexMap.
Please confirm the UI uses parent_path/is_nested to render a grouped fieldset as requested in issue #36.
991-1057: Regression test for additionalProperties: false flattening — nice coverage.Asserts field count, nesting flags, and absence of parent “metadata”. LGTM.
src-tauri/src/commands/files.rs (1)
743-751: Frontmatter rebuild + new tests — solid.Ordered keys + nested object/array YAML output look correct; roundtrip tests are valuable. After fixing the datetime issue, these should still pass.
Also applies to: 1763-1893
test/dummy-astro-project/.astro/settings.json (1)
3-3: Non-functional timestamp update.Nothing to review.
src/components/frontmatter/fields/FieldWrapper.test.tsx (1)
301-329: LGTM! Comprehensive test coverage for the new hideDefaultValue prop.The two new tests properly verify both the true and false cases for the hideDefaultValue prop, following the existing test patterns and ensuring the default value display behavior is correctly controlled.
test/dummy-astro-project/.astro/collections/articles.schema.json (1)
154-157: Verify the intentional addition of $schema as a data property.The
$schemaproperty is being added as a data field within the articles schema (insideproperties), which is unusual since$schemais typically used for JSON Schema metadata (already present at line 166). This would make$schemaa frontmatter field in articles.Please confirm this is intentional. If this is meant to reference a schema for article content, consider using a different name like
contentSchemaorschemaRefto avoid confusion with the JSON Schema$schemakeyword.test/dummy-astro-project/src/content/notes/copyedit-test.md (1)
5-7: LGTM! YAML formatting improvement.The tags field has been reformatted from inline array notation to multiline YAML block notation. Both formats are semantically equivalent, and this change aligns with the improved YAML frontmatter handling introduced in this PR.
src/components/frontmatter/FrontmatterPanel.tsx (1)
111-127: LGTM! Correctly filters out parent object keys when nested fields exist.The new filtering logic properly excludes parent keys (e.g., "metadata") from extraFields when the schema contains nested properties (e.g., "metadata.category", "metadata.priority"). This prevents the UI from showing a confusing input for the parent object itself, which directly addresses the issue described in the PR objectives.
test/dummy-astro-project/src/content.config.ts (1)
45-49: LGTM! Well-structured test case for object field handling.The metadata object schema provides a good test case with:
- Required nested field (category)
- Optional nested fields (priority, deadline)
- The entire object being optional
This aligns with the schema definitions in the JSON schema files and the test markdown content.
test/dummy-astro-project/src/content/notes/2024-03-15-object-field-test.md (1)
1-35: LGTM! Clear test case and documentation.This test file serves dual purposes effectively:
- Provides frontmatter with the metadata object for UI testing
- Documents the expected behavior for object field rendering
The frontmatter structure correctly matches the schema definitions, and the documentation clearly describes the intended UI behavior for object fields.
test/dummy-astro-project/.astro/collections/notes.schema.json (2)
46-76: LGTM! Well-defined metadata object structure.The metadata object provides an excellent test case for nested object handling with:
- Required nested field (category)
- Optional numeric field (priority)
- Optional date field with multiple formats (deadline)
- Strict constraint with
additionalProperties: falseThis structure aligns perfectly with the corresponding Zod schema in content.config.ts and the test markdown file.
77-79: Verify the intentional addition of $schema as a data property.Similar to the articles schema,
$schemais being added as a data field within the notes schema properties, which is unusual since$schematypically refers to JSON Schema metadata (already present at line 88). This would make$schemaa frontmatter field in notes.Please confirm this is intentional. If you need to reference a schema for note content, consider using a different property name like
contentSchemaorschemaRefto avoid confusion with the JSON Schema$schemakeyword.
| /// Serialize a value to YAML format with proper indentation | ||
| fn serialize_value_to_yaml(value: &Value, indent_level: usize) -> String { | ||
| let indent = " ".repeat(indent_level); | ||
|
|
||
| match value { | ||
| Value::String(s) => { | ||
| // Convert ISO datetime strings to date-only format | ||
| if s.len() > 10 | ||
| && s.contains('T') | ||
| && (s.ends_with('Z') || s.contains('+') || s.contains('-')) | ||
| { | ||
| // This looks like an ISO datetime string, extract just the date part | ||
| if let Some(date_part) = s.split('T').next() { | ||
| if date_part.len() == 10 && date_part.matches('-').count() == 2 { | ||
| return date_part.to_string(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if s.len() == 10 | ||
| && s.matches('-').count() == 2 | ||
| && s.chars().all(|c| c.is_ascii_digit() || c == '-') | ||
| { | ||
| // This looks like a date string (YYYY-MM-DD), don't quote it | ||
| s.clone() | ||
| } else if s.contains(' ') || s.contains(':') || s.contains('\n') { | ||
| // Quote strings that contain special characters or spaces | ||
| format!("\"{}\"", s.replace('"', "\\\"")) | ||
| } else { | ||
| s.clone() | ||
| } | ||
| } | ||
| Value::Bool(b) => b.to_string(), | ||
| Value::Number(n) => n.to_string(), | ||
| Value::Array(arr) => { | ||
| // Format array as YAML array | ||
| if arr.is_empty() { | ||
| "[]".to_string() | ||
| } else { | ||
| let mut array_str = String::new(); | ||
| for item in arr { | ||
| let item_str = match item { | ||
| Value::String(s) => s.clone(), | ||
| _ => serialize_value_to_yaml(item, 0), | ||
| }; | ||
| array_str.push_str(&format!("\n{indent} - {item_str}")); | ||
| } | ||
| array_str | ||
| } | ||
| } | ||
| Value::Object(obj) => { | ||
| // Format object as nested YAML | ||
| let mut object_str = String::new(); | ||
| for (key, val) in obj { | ||
| let val_str = serialize_value_to_yaml(val, indent_level + 1); | ||
| // Check if value needs to be on next line (objects/arrays) | ||
| if matches!(val, Value::Object(_) | Value::Array(_)) { | ||
| object_str.push_str(&format!("\n{indent} {key}:{val_str}")); | ||
| } else { | ||
| object_str.push_str(&format!("\n{indent} {key}: {val_str}")); | ||
| } | ||
| } | ||
| object_str | ||
| } | ||
| Value::Null => "null".to_string(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: ISO datetime strings are being truncated to dates during serialization.
serialize_value_to_yaml strips time/timezone (e.g., 2025-10-21T10:05:00Z → 2025-10-21), causing irreversible data loss on save. Serialization must be lossless unless the schema says “date” (not available here).
- Value::String(s) => {
- // Convert ISO datetime strings to date-only format
- if s.len() > 10
- && s.contains('T')
- && (s.ends_with('Z') || s.contains('+') || s.contains('-'))
- {
- // This looks like an ISO datetime string, extract just the date part
- if let Some(date_part) = s.split('T').next() {
- if date_part.len() == 10 && date_part.matches('-').count() == 2 {
- return date_part.to_string();
- }
- }
- }
-
- if s.len() == 10
+ Value::String(s) => {
+ // Preserve strings verbatim by default; only unquote safe YYYY-MM-DD dates
+ if s.len() == 10
&& s.matches('-').count() == 2
&& s.chars().all(|c| c.is_ascii_digit() || c == '-')
{
// This looks like a date string (YYYY-MM-DD), don't quote it
s.clone()
} else if s.contains(' ') || s.contains(':') || s.contains('\n') {
// Quote strings that contain special characters or spaces
format!("\"{}\"", s.replace('"', "\\\""))
} else {
s.clone()
}
}Add a test ensuring a frontmatter field like updatedAt: "2025-10-21T10:05:00Z" roundtrips unchanged.
1 participant
So there was kind of support for this before, but it was broken. Closes #36
Summary by CodeRabbit
New Features
Bug Fixes
Documentation