[034-focus-invariants-in-domain]: Move Focus/Task invariants into domain#40
Merged
archae0pteryx merged 3 commits intomainfrom May 5, 2026
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe PR introduces domain-level validation for empty and whitespace-only focus titles and task text by creating a ChangesDomain Validation & Error Handling
Sequence Diagram(No diagram generated—the changes represent straightforward validation logic movement and error mapping without introducing new multi-component control flows that would benefit from visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/commands/src/focus.rs`:
- Around line 42-43: The change to validation flow in NewFocus::new (used in the
focus creation command) can let DomainError map to CommandError::BadRequest, but
there are no regression tests asserting that a blank title or blank task yields
a CommandError::BadRequest; add tests that call the command handler (the
function that constructs NewFocus via NewFocus::new and then executes the
command) with input containing an empty title and with empty task(s), and assert
the returned error is CommandError::BadRequest (and optionally check the error
message matches the original DomainError); target the test(s) to exercise the
code paths touched around NewFocus::new and the focus creation command so
API-level error semantics remain pinned.
In `@crates/domain/src/proposal.rs`:
- Around line 63-78: The NewFocus constructor enforces a non-empty title but the
struct is still instantiable directly, so make the invariant non-bypassable by
making NewFocus's fields private (e.g., remove pub from fields) and exposing
only NewFocus::new for creation (returning Result and using
DomainError::EmptyTitle on empty titles); also update any deserialization/deser
path (e.g., impl Deserialize for NewFocus or serde attributes) to call
NewFocus::new during deserialization and adjust places that previously
constructed NewFocus directly (including tests) to use the constructor, ensuring
timer_preset remains handled via the constructor or appropriate builder methods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c443f419-e7a7-4f4e-8c44-f14d5e6c43c4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlcrates/commands/src/error.rscrates/commands/src/focus.rscrates/domain/Cargo.tomlcrates/domain/src/error.rscrates/domain/src/lib.rscrates/domain/src/proposal.rscrates/domain/src/task_text.rs
archae0pteryx
force-pushed
the
chore/focus-invariants-in-domain
branch
from
5587859 to
57970b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/domain/src/proposal.rs (1)
17-32:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
NewFocus::newdoes not make the title invariant non-bypassable.Even with this constructor, invalid domain state is still possible because
NewFocusremains publicly constructible andDeserializecan instantiate it without callingnew. That means the invariant is not enforced “once in domain” yet.Suggested direction
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct NewFocus { - pub title: String, - pub description: String, + title: String, + description: String, #[serde(skip_serializing_if = "Option::is_none", default)] - pub timer_preset: Option<TimerPreset>, + timer_preset: Option<TimerPreset>, } impl NewFocus { pub fn new( title: impl Into<String>, description: impl Into<String>, ) -> Result<Self, DomainError> { let title = title.into(); if title.trim().is_empty() { return Err(DomainError::EmptyTitle); } Ok(Self { title, description: description.into(), timer_preset: None, }) } + + pub fn title(&self) -> &str { &self.title } + pub fn description(&self) -> &str { &self.description } + pub fn timer_preset(&self) -> Option<TimerPreset> { self.timer_preset } + pub fn with_timer_preset(mut self, timer_preset: Option<TimerPreset>) -> Self { + self.timer_preset = timer_preset; + self + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/domain/src/proposal.rs` around lines 17 - 32, NewFocus currently can be instantiated bypassing its validation because Deserialize can construct it directly; fix by making NewFocus's fields private (remove pub) so external code/serde can't build invalid instances, and implement a custom Deserialize (or TryFrom + #[serde(deserialize_with = "...")]) that calls NewFocus::new and returns a deserialization error on invalid titles; update any public API to expose only NewFocus::new (or TryFrom) for creation and ensure types referenced (NewFocus, NewFocus::new) are used in the custom deserializer to enforce the invariant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/commands/src/focus.rs`:
- Around line 133-145: The test append_task_blank_text_returns_bad_request
hardcodes "real-focus" as the focus id; instead capture the created focus's id
from the create_focus call and use that when calling append_task. Change the
test to call create_focus and bind its Ok value (the created Focus/ID from
create_focus(CreateFocusInput { ... })), then pass that returned id/slug into
commands.append_task(...) rather than the literal "real-focus" so the test does
not depend on a specific id generation strategy; keep the rest of the assertion
the same.
---
Duplicate comments:
In `@crates/domain/src/proposal.rs`:
- Around line 17-32: NewFocus currently can be instantiated bypassing its
validation because Deserialize can construct it directly; fix by making
NewFocus's fields private (remove pub) so external code/serde can't build
invalid instances, and implement a custom Deserialize (or TryFrom +
#[serde(deserialize_with = "...")]) that calls NewFocus::new and returns a
deserialization error on invalid titles; update any public API to expose only
NewFocus::new (or TryFrom) for creation and ensure types referenced (NewFocus,
NewFocus::new) are used in the custom deserializer to enforce the invariant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ff8551ed-0c86-441c-9b48-9a15d8ce4288
📒 Files selected for processing (7)
Taskfile.yamlcrates/commands/src/error.rscrates/commands/src/focus.rscrates/domain/src/error.rscrates/domain/src/focus.rscrates/domain/src/lib.rscrates/domain/src/proposal.rs
💤 Files with no reviewable changes (1)
- Taskfile.yaml
1 task
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/domain/src/proposal.rs (1)
192-205: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueTests bypass
NewFocus::newvalidation by using struct literals.These tests construct
NewFocusvia struct literals (which works only within this module due to Rust privacy). While the test at lines 192-205 intentionally testsProposal::validateas a defense-in-depth layer, using struct literals is inconsistent with the PR's goal of routing all construction throughNewFocus::new.Consider updating these tests to use
NewFocus::new(...)or document why direct struct construction is intentional here.♻️ Suggested refactor for consistency
For
round_trip_preserves_kind(lines 261-271):fn round_trip_preserves_kind() { let original = ok_proposal(ProposalKind::NewFocus { - new_focus: NewFocus { - title: "T".into(), - description: "D".into(), - timer_preset: None, - }, + new_focus: NewFocus::new("T", "D").unwrap(), });For
new_focus_requires_title(lines 192-205), if the intent is to testProposal::validateas a fallback layer, add a comment explaining this design choice.Also applies to: 261-271
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/domain/src/proposal.rs` around lines 192 - 205, The tests currently construct NewFocus via struct literals (e.g., in new_focus_requires_title and round_trip_preserves_kind) which bypass NewFocus::new validation; update those tests to call NewFocus::new(...) when creating NewFocus instances (and handle the Result) so construction always goes through NewFocus::new, or if the test intentionally exercises Proposal::validate as a defense-in-depth layer, add a concise comment above the test (naming the test like new_focus_requires_title) stating that direct struct construction is deliberate to test Proposal::validate rather than NewFocus::new; reference the NewFocus struct and Proposal::validate in the comment to make the intent explicit.
♻️ Duplicate comments (1)
crates/commands/src/focus.rs (1)
133-145:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest uses hardcoded slug instead of returned focus id.
Line 143 uses
"real-focus"as a hardcoded slug, which couples the test to the current slugification strategy. This was flagged in a previous review (marked as addressed), but the code still uses the hardcoded value.🛠️ Suggested fix
#[test] fn append_task_blank_text_returns_bad_request() { let (commands, _dir) = build_commands(0); - commands + let created = commands .create_focus(CreateFocusInput { title: "Real focus".into(), description: String::new(), timer_preset: None, }) .unwrap(); - let err = commands.append_task("real-focus", " ").unwrap_err(); + let err = commands.append_task(&created.id, " ").unwrap_err(); assert!(matches!(err, CommandError::BadRequest(_))); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/commands/src/focus.rs` around lines 133 - 145, The test currently hardcodes the slug "real-focus" when calling commands.append_task which couples the test to slugification; instead capture the value returned by commands.create_focus(CreateFocusInput { ... }).unwrap() (e.g. let focus = commands.create_focus(...).unwrap();) and pass the returned focus identifier (focus.id or focus.slug as provided by the create_focus return type) into commands.append_task so the test uses the actual created focus identifier rather than a hardcoded string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/domain/src/proposal.rs`:
- Around line 192-205: The tests currently construct NewFocus via struct
literals (e.g., in new_focus_requires_title and round_trip_preserves_kind) which
bypass NewFocus::new validation; update those tests to call NewFocus::new(...)
when creating NewFocus instances (and handle the Result) so construction always
goes through NewFocus::new, or if the test intentionally exercises
Proposal::validate as a defense-in-depth layer, add a concise comment above the
test (naming the test like new_focus_requires_title) stating that direct struct
construction is deliberate to test Proposal::validate rather than NewFocus::new;
reference the NewFocus struct and Proposal::validate in the comment to make the
intent explicit.
---
Duplicate comments:
In `@crates/commands/src/focus.rs`:
- Around line 133-145: The test currently hardcodes the slug "real-focus" when
calling commands.append_task which couples the test to slugification; instead
capture the value returned by commands.create_focus(CreateFocusInput { ...
}).unwrap() (e.g. let focus = commands.create_focus(...).unwrap();) and pass the
returned focus identifier (focus.id or focus.slug as provided by the
create_focus return type) into commands.append_task so the test uses the actual
created focus identifier rather than a hardcoded string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c5961e53-37c9-4d7a-bf4a-4976eed5a12b
📒 Files selected for processing (6)
crates/commands/src/focus.rscrates/commands/src/lifecycle.rscrates/commands/src/proposal.rscrates/domain/src/proposal.rscrates/storage/src/focus_store.rscrates/storage/src/proposals.rs
Closes issues/034-focus-invariants-in-domain.md Completion promise: Focus title and task text invariants are enforced once in crates/domain/ via typed constructors; no validation logic remains in the commands layer. - Add DomainError enum (EmptyTitle, EmptyTaskText) in domain crate - Add NewFocus::new validated constructor - Add TaskText newtype with validated new + as_str - Replace trim().is_empty() guards in Commands::create_focus and Commands::append_task; map DomainError -> CommandError::BadRequest - Drop stale ralph/ Taskfile include left over from dabed05 so task check runs again
Previously NewFocus had pub fields, so callers (including Deserialize) could construct invalid domain state without going through ::new(). The invariant only held by convention. Make fields private; expose accessors and a with_timer_preset builder. Implement Deserialize manually so wire-format input also routes through ::new() and rejects empty titles. CreateProposalInput now returns BadRequest when kind=new_focus is sent without a new_focus payload.
archae0pteryx
force-pushed
the
chore/focus-invariants-in-domain
branch
from
67069bc to
80e2804
Compare
9 tasks
This was referenced May 5, 2026
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.
Closes issues/034-focus-invariants-in-domain.md
Summary
DomainErrorenum (EmptyTitle,EmptyTaskText) incrates/domain/NewFocus::new(title, description)validated constructorTaskTextnewtype with validatednew+as_strtrim().is_empty()guards inCommands::create_focusandCommands::append_task; mapDomainError->CommandError::BadRequestralph/Taskfile include left over from dabed05 sotask checkruns againTest plan
task checkgreenSummary by CodeRabbit
New Features
Bug Fixes