-
-
Notifications
You must be signed in to change notification settings - Fork 1
RC 1.3.0 #51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RC 1.3.0 #51
Conversation
|
Warning Rate limit exceeded@strawmelonjuice has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds a new boolean field Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
♻️ Duplicate comments (1)
cynthia_websites_mini_client/src/cynthia_websites_mini_client/pottery/molds/cindy_simple.gleam (1)
355-360: Samemd:col-span[]class concern as incindy_dualThe post-meta class string here also contains
md:col-span[]. This appears to be the same probable typo mentioned in the dual layout; consider correcting both together to avoid inconsistent or unintended responsive behaviour.
🧹 Nitpick comments (7)
cynthia_websites_mini_server/src/cynthia_websites_mini_server/config.gleam (1)
544-546: Consider graceful error handling for directory creation.Using
let assert Ok(_)for directory creation will cause a panic if creation fails (e.g., due to permissions issues). Given that other file operations in this function use proper error handling, this should be consistent.- let assert Ok(_) = - simplifile.create_directory_all(process.cwd() <> "/content") - let assert Ok(_) = simplifile.create_directory_all(process.cwd() <> "/assets") + case simplifile.create_directory_all(process.cwd() <> "/content") { + Ok(_) -> Nil + Error(_) -> { + console.error("Error: Could not create content directory.") + process.exit(1) + } + } + case simplifile.create_directory_all(process.cwd() <> "/assets") { + Ok(_) -> Nil + Error(_) -> { + console.error("Error: Could not create assets directory.") + process.exit(1) + } + }cynthia_websites_mini_server/src/cynthia_websites_mini_server/config/v4.gleam (1)
30-37: Unreachable code afterprocess.exit(1).The
result.map_error(string.inspect)on line 36 is unreachable because the previousresult.map_errorblock callsprocess.exit(1)on error. This is dead code that adds confusion.use str <- promise.try_await( fs.read_file_sync(files.path_normalize(process.cwd() <> "/cynthia.toml")) |> result.map_error(fn(e) { - premixed.text_error_red("Error: Could not read cynthia.toml: " <> e) + console.error( + premixed.text_error_red("Error: Could not read cynthia.toml: " <> e), + ) process.exit(1) + e }) - |> result.map_error(string.inspect) |> promise.resolve(), )cynthia_websites_mini_client/src/cynthia_websites_mini_client/pottery.gleam (1)
27-27: Propagation ofhide_metadata_blockinto the template context looks correctThe PageData pattern and the additional
variablesentry are consistent with the newPageData(in_menus, hide_meta_block)shape and give the layouts exactly what they need. The only tiny nit is naming: JSON uses"hide_meta", the type useshide_meta_block, and the template key ishide_metadata_block, which might be a bit cognitive overhead later; consider aligning names or adding a short doc comment somewhere if this split is intentional.Also applies to: 42-45
cynthia_websites_mini_client/src/cynthia_websites_mini_client/pottery/molds/cindy_dual.gleam (1)
179-191: Hide-flag decoding and class helper are sound and backwards compatibleDecoding
hide_metadata_blockwith a default ofFalseand converting it into" hidden"/""is a clean way to toggle the metadata column without breaking existing content. Given the same logic exists incindy_simple.cindy_common, you might later want a small shared helper to avoid duplication, but it’s not urgent.cynthia_websites_mini_client/src/cynthia_websites_mini_client/contenttypes.gleam (1)
116-121: PageData schema extension and (de)serialisation look consistent and backwards compatibleAdding
hide_meta_block: Booland wiring it throughcontent_data_decoderwithoptional_field("hide_meta", False, decode.bool)plus the matching"hide_meta"encoder keeps older content valid while enabling the new flag. The only thing to keep in mind is that we now have three related names ("hide_meta"JSON,hide_meta_blocktype field,hide_metadata_blocktemplate key); if this grows further, a brief comment in the type or a small docs note might help future readers.Also applies to: 135-142, 161-166
cynthia_websites_mini_client/src/cynthia_websites_mini_client/pottery/molds/cindy_simple.gleam (2)
204-216: Hide-flag handling and application in the simple layout are coherentDecoding
hide_metadata_blockwith a default ofFalseand appending" hidden"/""to the post-meta container class matches the dual layout and gives a clear, non-breaking switch to hide the side block for PageData-driven pages. As incindy_dual, this duplication could be abstracted into a helper later if you find yourself tweaking the behaviour again.Also applies to: 355-360
219-219: Remove commented-out debug line to keep the template cleanThe commented
element.text(variables |> string.inspect)looks like leftover debugging; removing it would reduce noise around the layout markup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(1 hunks)cynthia_websites_mini_client/src/cynthia_websites_mini_client.gleam(2 hunks)cynthia_websites_mini_client/src/cynthia_websites_mini_client/contenttypes.gleam(3 hunks)cynthia_websites_mini_client/src/cynthia_websites_mini_client/pottery.gleam(2 hunks)cynthia_websites_mini_client/src/cynthia_websites_mini_client/pottery/molds/cindy_dual.gleam(2 hunks)cynthia_websites_mini_client/src/cynthia_websites_mini_client/pottery/molds/cindy_simple.gleam(3 hunks)cynthia_websites_mini_client/src/cynthia_websites_mini_client/view.gleam(5 hunks)cynthia_websites_mini_server/manifest.toml(3 hunks)cynthia_websites_mini_server/src/cynthia_websites_mini_server/config.gleam(11 hunks)cynthia_websites_mini_server/src/cynthia_websites_mini_server/config/v4.gleam(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-10T13:40:49.897Z
Learnt from: strawmelonjuice
Repo: CynthiaWebsiteEngine/Mini PR: 33
File: cynthia_websites_mini_server/src/cynthia_websites_mini_server/config.gleam:896-899
Timestamp: 2025-07-10T13:40:49.897Z
Learning: In CynthiaWebsiteEngine Mini, the user prefers to avoid requiring external dependencies like Pandoc during initial setup to maintain a smooth out-of-the-box experience, even if it means accepting some compatibility risks with Markdown/Djot parsing differences.
Applied to files:
README.mdcynthia_websites_mini_server/src/cynthia_websites_mini_server/config.gleam
🔇 Additional comments (13)
README.md (1)
76-76: Configuration filename updated consistently with codebase migration.The example configuration path has been correctly updated to
./cynthia.tomlto reflect the new config filename standard across the project.cynthia_websites_mini_server/manifest.toml (4)
21-21: Verify thegleam_httpcversion downgrade.Line 21 downgrades
gleam_httpcfrom 4.2.1 to 4.1.1. This is unusual and could indicate a compatibility constraint or regression. Please confirm this downgrade was intentional and does not introduce compatibility issues with the HTTP client usage in the codebase.
27-27: Verifygleam_time1.6.0 compatibility.Line 27 upgrades
gleam_timefrom 1.3.0 to 1.6.0, a significant version jump. Please ensure the new version is compatible with time-related operations in the config parsing or rendering pipelines, particularly given the new config v4 parser mentioned in the PR summary.
10-10: Client library version bump aligns with feature propagation.Line 10 updates
cynthia_websites_mini_clientto 1.2.0-rc2. This patch version aligns with the introduction of thehide_meta_blockfield in PageData and its propagation through rendering paths as described in the PR summary.
32-32: Coordinated updates to template and parsing dependencies.Lines 32, 34, and 45 update
houdini(1.1.0→1.2.0),jot(5.0.0→5.0.1), andsplitter(1.0.0→1.2.0). These appear well-coordinated sincejotdepends on bothhoudiniandsplitter. The newjot5.0.1 with updated dependencies should support the new config v4 parser introduced in this release.Also applies to: 34-34, 45-45
cynthia_websites_mini_server/src/cynthia_websites_mini_server/config.gleam (3)
1-5: LGTM!Import addition for the v4 config parser is correctly placed with other server-side imports.
129-166: LGTM!The error helper functions provide clear, actionable error messages. The
parse_config_formatfunction cleanly uses monadic error handling withresult.try.
586-644: LGTM!The
PageDatainstantiations correctly include the newhide_meta_blockfield with appropriate boolean values for each content type (e.g.,Truefor landing pages and post lists,Falsefor regular pages).cynthia_websites_mini_server/src/cynthia_websites_mini_server/config/v4.gleam (3)
56-125: LGTM!The required field parsing follows a consistent pattern with clear error messages indicating which config path is missing.
126-179: LGTM!Optional field handling is well-implemented with sensible defaults (git integration enabled by default, crawlable context disabled by default).
415-456: LGTM!The final assembly correctly collates all parsed values into the config object. The
let asserton line 433 is safe due to the preceding error filtering logic.cynthia_websites_mini_client/src/cynthia_websites_mini_client.gleam (1)
571-584: Menu computation correctly ignores the new hide flagUpdating the matches to
PageData(soms, _)/PageData(m, _)keeps menu derivation based solely onin_menuswhile safely discarding the hide flag, so existing menu behaviour is preserved. No issues spotted here.Also applies to: 587-617
cynthia_websites_mini_client/src/cynthia_websites_mini_client/view.gleam (1)
28-36: PageData flag choices per route look intentional; worth a quick UX sanity checkThe new
PageData([], Bool)constructions are consistent with the type order and with how you use the flag in the layouts: 404 and tag postlists hide the side metadata block, while category, search, and all-posts lists leave it visible. From a code perspective this is fine; just confirm that this matrix ofTrue/Falsematches the UX you actually want for each of those special routes.Also applies to: 54-67, 69-85, 87-103, 105-117
cynthia_websites_mini_client/src/cynthia_websites_mini_client/pottery/molds/cindy_dual.gleam
Show resolved
Hide resolved
cynthia_websites_mini_server/src/cynthia_websites_mini_server/config.gleam
Show resolved
Hide resolved
cynthia_websites_mini_server/src/cynthia_websites_mini_server/config.gleam
Show resolved
Hide resolved
cynthia_websites_mini_server/src/cynthia_websites_mini_server/config/v4.gleam
Outdated
Show resolved
Hide resolved
cynthia_websites_mini_server/src/cynthia_websites_mini_server/config/v4.gleam
Show resolved
Hide resolved
cynthia_websites_mini_server/src/cynthia_websites_mini_server/config/v4.gleam
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cynthia_websites_mini_server/src/cynthia_websites_mini_server/config.gleam (1)
108-119: Avoid silently converting config read failures into an empty string
global_conf_content_syncstill usessimplifile.read(global_conf_filepath) |> result.unwrap("")(Lines 108–110). If the read fails, this turns any IO error into an empty string, soparse_config_formatthen fails with a generic “Cannot properly read config.edition…” message instead of the real “failed to read cynthia.toml: ”. This was flagged previously and remains unresolved.It would be better to handle the
Resultexplicitly here, log the underlying error, and exit before attempting to parse:- let global_conf_content_sync = - simplifile.read(global_conf_filepath) |> result.unwrap("") + let global_conf_content_sync = + case simplifile.read(global_conf_filepath) { + Ok(content) -> content + Error(e) -> { + console.error( + "Error: Could not read " <> global_conf_filepath <> ": " <> e, + ) + process.exit(1) + panic as "We should not reach here" + } + }This keeps the new config‑format detection while surfacing real filesystem issues clearly.
🧹 Nitpick comments (3)
cynthia_websites_mini_client/src/cynthia_websites_mini_client/pottery/molds/cindy_dual.gleam (1)
360-361: Conditionalhiddenclass concatenation is correct; consider small readability tweakAppending
" hidden"(with leading space) or""to the static Tailwind class string is correct and yields valid class names in both cases. If you touch this again, you might consider extracting a helper that builds the full class string from the flag to make the conditional layout logic slightly clearer, but it’s not strictly necessary.cynthia_websites_mini_server/src/cynthia_websites_mini_server/config.gleam (2)
48-105: Legacy config auto‑upgrade flow looks good; minor error‑reporting polish possibleThe cynthia‑mini.toml → cynthia.toml upgrade logic and rename flow are sound, and the user‑facing warning text is clear. One small improvement: in the
True, Error(_)branch (Lines 93–100), you currently log a very generic message and discard the underlying IO error. MatchingError(e)and includingstring.inspect(e)in the log would make debugging failed upgrades easier, similar to how other error paths now include details.This is non‑blocking and can be deferred.
133-169: Minor readability nits in config‑format error helpersFunctionally this section works, but there are a couple of small clarity tweaks you might consider:
In
promise_error_unknown_config_format(Lines 137–142), the pipeline/concat chain"Config version " <> version |> int.to_string()relies on
|>precedence to do the right thing. For readability, you may prefer the more explicit"Config version " <> int.to_string(version)or
"Config version " <> (version |> int.to_string()).For symmetry with
promise_error_unknown_config_format, you could givepromise_error_cannot_read_config_formatan explicit polymorphic return type:fn promise_error_cannot_read_config_format() -> Promise(Result(a, String)) { promise.resolve(Error( "Cannot properly read config.edition and/or config.version, " <> "Cynthia doesn't know how to parse this file anymore!", )) }This keeps the helper generic and self‑documenting.
These are style/readability only; behaviour is fine as‑is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cynthia_websites_mini_client/src/cynthia_websites_mini_client/pottery/molds/cindy_dual.gleam(2 hunks)cynthia_websites_mini_client/src/cynthia_websites_mini_client/pottery/molds/cindy_simple.gleam(3 hunks)cynthia_websites_mini_server/src/cynthia_websites_mini_server/config.gleam(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cynthia_websites_mini_client/src/cynthia_websites_mini_client/pottery/molds/cindy_simple.gleam
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-10T13:40:49.897Z
Learnt from: strawmelonjuice
Repo: CynthiaWebsiteEngine/Mini PR: 33
File: cynthia_websites_mini_server/src/cynthia_websites_mini_server/config.gleam:896-899
Timestamp: 2025-07-10T13:40:49.897Z
Learning: In CynthiaWebsiteEngine Mini, the user prefers to avoid requiring external dependencies like Pandoc during initial setup to maintain a smooth out-of-the-box experience, even if it means accepting some compatibility risks with Markdown/Djot parsing differences.
Applied to files:
cynthia_websites_mini_server/src/cynthia_websites_mini_server/config.gleam
🔇 Additional comments (3)
cynthia_websites_mini_client/src/cynthia_websites_mini_client/pottery/molds/cindy_dual.gleam (1)
179-191: Boolean decode and fallback forhide_metadata_blocklook solidDecoding the flag from
variableswith a defaultFalse, and unwrapping failures toFalse, is robust and consistent with the rest of this module; this will not break rendering if the key is missing or mis‑typed. The derivedhide_metadata_block_classonlykeeps the downstream usage simple and readable.cynthia_websites_mini_server/src/cynthia_websites_mini_server/config.gleam (2)
430-533: Newbrand_new_configtemplate is coherent and matches the v4 config modelThe
brand_new_configTOML string looks structurally correct and aligns with the new “universal”cynthia.tomllayout:
config.edition = "mini"/config.version = 4are set up front, soparse_config_formatcan reliably route to the v4 parser.- The comments give a clear, guided starting point for users (global/server/integrations/variables/posts).
ownit_templateis nicely documented and the triple‑quote escaping (\"\"\") appears correct for TOML.Also, all bundled sample content here is
.dj, so initialinitcfgruns don’t force users to have Pandoc installed; only folks who add Markdown themselves will hit the Pandoc path later, which matches your earlier preference to keep OOTB setup dependency‑light. Based on learnings, this is a solid direction.
580-652: Sample content and newPageDatausages look consistentThe updated example content in
initcfgcorrectly threads the newPageDatashape:
contenttypes.PageData([2], False)forhangers.djand[1], Falseforthemes.dj/index.djmake sense as “show meta block, appear in specific menus”.- For the synthetic
postslisting page,data: contenttypes.PageData(in_menus: [1], hide_meta_block: True)(Line 648) is a good default so the meta block doesn’t show on the special!permalink listing.The
ext_itemfor the themes page now targets a.djsource in the Mini‑docs repo, which keeps the default content consistent with the Djot‑first approach and avoids extra Markdown→Djot conversion at init time.No issues spotted here.
Issues squashed with this release:
Summary by CodeRabbit
New Features
Documentation
Chores
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.