Feat/new realese update - #3
Merged
Merged
Conversation
docs: add initial design system documentation in DESIGN.md
…nfiguration file discovery
Reviewer's GuideImplements auto-discovery for the Mailify TOML config file (with platform-aware search paths and tests), refactors the Docker build to use cargo-chef for better caching, reworks the CI Docker workflow into per-arch builds plus a manifest aggregation job, bumps the workspace version, and adds a comprehensive design system document. Flow diagram for TOML config auto-discoveryflowchart TD
Start([Start config discovery]) --> CheckEnv{MAILIFY_CONFIG set?}
CheckEnv -- Yes --> EnvPath["Use path from MAILIFY_CONFIG"]
EnvPath --> ExistsEnv{File exists?}
ExistsEnv -- Yes --> UseEnv["Return MAILIFY_CONFIG path"]
ExistsEnv -- No --> CheckCwd
CheckEnv -- No --> CheckCwd{{"Mailify.toml in CWD?"}}
CheckCwd -- Yes --> UseCwd["Return ./Mailify.toml"]
CheckCwd -- No --> UserDir{user_config_dir available?}
UserDir -- Yes --> UserPath["user_config_dir/mailify/config.toml"]
UserPath --> ExistsUser{File exists?}
ExistsUser -- Yes --> UseUser["Return per-user config path"]
ExistsUser -- No --> CheckEtc
UserDir -- No --> CheckEtc
subgraph UnixOnly[Unix-only step]
CheckEtc{{"/etc/mailify/config.toml exists?"}}
end
CheckEtc -- Yes --> UseEtc["Return /etc/mailify/config.toml"]
CheckEtc -- No --> None["Return None (no config file)"]
UseEnv --> End([End])
UseCwd --> End
UseUser --> End
UseEtc --> End
None --> End
Flow diagram for Dockerfile multi-stage build with cargo-chefflowchart LR
A[bun-builder\nbuild templates-parser] --> ChefBase[chef stage\ninstall cargo-chef]
ChefBase --> Planner[planner stage\ncopy manifests + crates\n`cargo chef prepare`]
Planner --> Cacher[cacher stage\n`cargo chef cook --release`\ncache deps]
Cacher --> Builder[rs-builder stage\ncopy target + cargo dirs\ncopy sources\n`cargo build --release --bin mailify`]
Builder --> Final[final image stage\ncopy `mailify` binary\ncopy built templates\nset entrypoint]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The test helpers mutate process environment variables inside
unsafeblocks, which is unnecessary and can lead to flaky, racy tests when run in parallel—consider encapsulating env save/restore in a safe helper (or using a crate liketemp_env) and marking these tests as serial if needed. config_candidateseagerly allocates aVec<PathBuf>just to scan for the first existing file; you could simplify and avoid allocation by returning an iterator (e.g.impl Iterator<Item = PathBuf>) and lettingdiscover_config_filechain candidates lazily.- In the CI workflow the
docker-buildjob exposesoutputs.imagebut the finaldocker-manifestjob recomputes tags via a separatedocker/metadata-actionand never uses that output—either wire theimageoutput through or drop it to avoid confusion about which metadata source is authoritative.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The test helpers mutate process environment variables inside `unsafe` blocks, which is unnecessary and can lead to flaky, racy tests when run in parallel—consider encapsulating env save/restore in a safe helper (or using a crate like `temp_env`) and marking these tests as serial if needed.
- `config_candidates` eagerly allocates a `Vec<PathBuf>` just to scan for the first existing file; you could simplify and avoid allocation by returning an iterator (e.g. `impl Iterator<Item = PathBuf>`) and letting `discover_config_file` chain candidates lazily.
- In the CI workflow the `docker-build` job exposes `outputs.image` but the final `docker-manifest` job recomputes tags via a separate `docker/metadata-action` and never uses that output—either wire the `image` output through or drop it to avoid confusion about which metadata source is authoritative.
## Individual Comments
### Comment 1
<location path="crates/mailify-config/src/lib.rs" line_range="234-236" />
<code_context>
+fn config_candidates() -> Vec<PathBuf> {
+ let mut out = Vec::new();
+
+ if let Ok(explicit) = std::env::var("MAILIFY_CONFIG") {
+ out.push(PathBuf::from(explicit));
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Empty `MAILIFY_CONFIG` values are treated as a path and become `.` implicitly.
When `MAILIFY_CONFIG` is set to an empty string, `PathBuf::from("")` becomes `"."`, so the current directory is added as a candidate and then `is_file()` is called on it. That’s a surprising interpretation of an empty override. Consider skipping empty values explicitly (e.g. check `!explicit.is_empty()` before pushing) so the env var only acts as a file path when it’s non-empty.
```suggestion
if let Ok(explicit) = std::env::var("MAILIFY_CONFIG") {
if !explicit.is_empty() {
out.push(PathBuf::from(explicit));
}
}
```
</issue_to_address>
### Comment 2
<location path="docker/Dockerfile" line_range="16-17" />
<code_context>
RUN apt-get update && apt-get install -y --no-install-recommends \
pkg-config libssl-dev ca-certificates curl unzip build-essential && rm -rf /var/lib/apt/lists/*
-
+RUN --mount=type=cache,target=/usr/local/cargo/registry \
+ cargo install cargo-chef --locked --version ^0.1
WORKDIR /w
</code_context>
<issue_to_address>
**suggestion:** The `cargo-chef` version is specified as a semver range, which can hurt build reproducibility.
Using `--version ^0.1` lets `cargo-chef` float to newer minor versions, which can change behavior over time and invalidate cached Docker layers. For more deterministic builds, consider pinning an exact version (e.g. `--version 0.1.XX`) and updating it explicitly when needed.
```suggestion
RUN --mount=type=cache,target=/usr/local/cargo/registry \
cargo install cargo-chef --locked --version 0.1.67
```
</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
+234
to
+236
| if let Ok(explicit) = std::env::var("MAILIFY_CONFIG") { | ||
| out.push(PathBuf::from(explicit)); | ||
| } |
There was a problem hiding this comment.
suggestion: Empty MAILIFY_CONFIG values are treated as a path and become . implicitly.
When MAILIFY_CONFIG is set to an empty string, PathBuf::from("") becomes ".", so the current directory is added as a candidate and then is_file() is called on it. That’s a surprising interpretation of an empty override. Consider skipping empty values explicitly (e.g. check !explicit.is_empty() before pushing) so the env var only acts as a file path when it’s non-empty.
Suggested change
| if let Ok(explicit) = std::env::var("MAILIFY_CONFIG") { | |
| out.push(PathBuf::from(explicit)); | |
| } | |
| if let Ok(explicit) = std::env::var("MAILIFY_CONFIG") { | |
| if !explicit.is_empty() { | |
| out.push(PathBuf::from(explicit)); | |
| } | |
| } |
Comment on lines
+16
to
+17
| RUN --mount=type=cache,target=/usr/local/cargo/registry \ | ||
| cargo install cargo-chef --locked --version ^0.1 |
There was a problem hiding this comment.
suggestion: The cargo-chef version is specified as a semver range, which can hurt build reproducibility.
Using --version ^0.1 lets cargo-chef float to newer minor versions, which can change behavior over time and invalidate cached Docker layers. For more deterministic builds, consider pinning an exact version (e.g. --version 0.1.XX) and updating it explicitly when needed.
Suggested change
| RUN --mount=type=cache,target=/usr/local/cargo/registry \ | |
| cargo install cargo-chef --locked --version ^0.1 | |
| RUN --mount=type=cache,target=/usr/local/cargo/registry \ | |
| cargo install cargo-chef --locked --version 0.1.67 |
- Created configuration reference documentation detailing all config options, their types, defaults, and purposes. - Added HTTP API documentation outlining all endpoints, authentication requirements, and request/response formats. - Introduced a template contract document explaining the directory layout and rendering conventions for templates. - Compiled a troubleshooting guide covering common errors, debugging tactics, and FAQs to assist users in resolving issues. - Implemented installation scripts for both Windows (install.ps1) and POSIX systems (install.sh) to simplify the setup process. - Added a logo image to the static assets for branding purposes.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Introduce auto-discovered configuration file loading, improve Rust build caching and multi-arch Docker publishing, bump workspace version, and add a formal design system document.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests: