fix: cross-module violations, Excalidraw diagrams, wireframe restructure#42
Merged
phuongnse merged 22 commits intoMay 19, 2026
Merged
Conversation
…malization FormRepository.IsReferencedByWorkflowAsync and WorkflowEngine.WorkflowDefinitionReader were querying workflow_definitions (WorkflowBuilder's table) directly from their own DbContexts. Both now use local read models maintained via Wolverine handlers. - Add WorkflowActiveStatus aggregate + EF config + migration in WorkflowEngine - Add FormWorkflowReference read model + EF config + migration in FormBuilder - Add Wolverine handlers in both modules consuming WorkflowPublished/Archived/Unarchived - Register Infrastructure assemblies in Program.cs opts.Discovery.IncludeAssembly - Add WorkflowBuilder.Domain project references to WE and FB Infrastructure projects - Fix test DB isolation: FormBuilder and WorkflowEngine each get their own test DB - Update 2 infrastructure tests to seed new read model tables instead of old raw SQL - Update CLAUDE.md with "No DbSet = stop signal, not raw SQL invitation" rule Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All .puml and .png files removed. Replaced with .excalidraw (source) and
.svg (generated preview) for all 9 diagrams across docs/diagrams/ and
docs/epics/E0{N}-name/diagrams/.
- Add docs/diagrams/generate-diagrams.mjs — Node.js ESM script generating all
9 Excalidraw JSON files (3 top-level + 6 epic-level)
- Rewrite docs/scripts/generate-diagrams.ps1 to scan .excalidraw recursively
and POST to Kroki.io /excalidraw/svg (was PlantUML /plantuml/png)
- Generate .svg for all 9 diagrams via Kroki
- Update docs/README.md, docs/ARCHITECTURE.md, all epic READMEs to link .svg
- Update CLAUDE.md: "Never use PlantUML — all diagrams are Excalidraw"
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Using strokeColor:"transparent" and strokeWidth:0 in entityBox caused Excalidraw to silently blank E03, E04, and E05 diagrams. Fixed by using strokeColor:headerBg (fill color = border color = invisible border) on the header overlay rectangle. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
entityBox returns midBottom/midTop as {x,y} objects, but arrow calls
were referencing .bottom and .top (non-existent) instead of .midBottom.y
and .midTop.y. JSON.stringify converts undefined to null, causing
Excalidraw to render arrows with null points and crash the diagram.
Affected: data-model (E03), workflow-model (E04), form-model (E05).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rams Switch all elements to roughness:1 and fontFamily:1 (Virgil) to match the hand-drawn feel of the wireframes. Add routedArrow() helper for multi-waypoint L/Z-shaped paths and replace all diagonal arrows that crossed boxes or text: E03 (dcf→ftype, fld→dcd, rec→mdl), E04 (wfd→trans above step box, step→pg), E05 (fdef→ffield, ffield→fft, ftask→fsub), and systemContext (EndUser→APIServer). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Diagrams were generated from memory, not from reading source code. Verified against actual .cs files and corrected: E03 — Data Model: - Rename ModelDefinition→DataModel, DataClassDefinition→DataClass, Record→DataRecord - Remove DataClassField (doesn't exist; DataClass reuses FieldDefinition) - Fix FieldType enum: JSON→Json - Correct field names: required→isRequired, add helpText, isSystem - Add missing audit fields: organizationId, createdBy, deletedAt E04 — Workflow Model: - Rename StepDefinition→WorkflowStep; remove slug, version, positionX/Y - Replace TriggerConfig entity with WorkflowTrigger «value object» (no id, no workflowId — just type + config) - Replace Transition entity with StepTransition «value object» (fromStepId, toStepId, label only) - Remove ParallelGroup and JoinType (not implemented) - Add Start and End to StepType enum - Triggers is now 1--* (list, not 1--1) - Add «value» stereotype to value object boxes E05 — Form Model: - Remove FormSection (doesn't exist) - Rename FormFieldDefinition→FormField; FormTask→FormSubmission - FormSubmission is the single aggregate (task + submission merged) - Fix FormFieldType: Checkbox→Section; fix order - Fix status enum: FormTaskStatus→FormSubmissionStatus, Completed→Submitted - Correct assignee: single assigneeId→assigneeUserId + assigneeRoleId Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…in code" This reverts commit 33b5360.
- system-context: complete layout rewrite — proper column separation, routed arrows for actors, clear platform boundary at 600px wide - container: fix 10px-wide invisible DB arrows (now 60px from x=810→870), increase module row gap from 30px to 65px, fix module column spacing - module-overview: fix bogus downward arrow at x=880 (was pointing to empty space; FormBuilder is above the event bus, arrow now goes UP), widen SharedKernel box to span all 4 row-1 modules, increase gap between event bus and row-2 modules (85px), add 15px badge clearance Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…reframes/
Move docs/wireframes/E0X-*/ → docs/epics/E0X-*/wireframes/ so each epic
folder is self-contained (features/ + diagrams/ + wireframes/).
- git mv all 6 epic wireframe folders (50 files, history preserved)
- Update 28 feature file links: ../../../wireframes/E0X-name/screen →
../wireframes/screen (shorter, no need to repeat the epic name)
- Update generate-screens.mjs write() to route E0X-* paths to
docs/epics/{epic}/wireframes/ automatically — no call-site changes needed
- Update generate-wireframes.ps1 to scan both docs/wireframes/ (shared/
template) and docs/epics/*/wireframes/
- Update CLAUDE.md wireframe convention, docs/README.md, playbooks/wireframes.md
Shared files (components.mjs, generate-*.mjs, _template, _shared/) remain
in docs/wireframes/ as they are not epic-specific.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
app-shell is a layout reference template, not a screen — co-locate it
with _template.excalidraw in docs/wireframes/ root. Remove now-empty
_shared/ folder.
- git mv _shared/app-shell.{excalidraw,svg} → wireframes root
- Update write() call in generate-screens.mjs
- Update CLAUDE.md, playbooks/wireframes.md screen inventory
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces PlantUML with Excalidraw assets and SVG pipeline; tightens docs/process rules; adds diagram-generation scripts; introduces FormBuilder API endpoints, strongly-typed request shapes and JSON converters; adds FormBuilder and WorkflowEngine read models, EF mappings, migrations, and Wolverine event handlers; updates tests and test fixture isolation. ChangesSingle Cohort
Estimated code review effort: Possibly related PRs:
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/epics/E05-form-builder/README.md (1)
79-85:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate Implementation Status to reflect completed API layer and corrected Infrastructure description.
The Implementation Status table is inconsistent with the code changes in this PR:
API layer: The stack context shows FormBuilder API endpoints (Layer 7: FormEndpoints.cs with full CRUD) and integration tests (Layer 18: FormEndpointTests.cs) were implemented in this PR, but the status still shows "⏳ Pending".
Infrastructure description: The note mentions "cross-module IsReferencedByWorkflowAsync JSONB query", but this PR replaced that cross-module query with a local
FormWorkflowReferenceread model synchronized via Wolverine handlers (Layers 10-11). The description should reflect this change.As per coding guidelines, the Epic README Implementation Status table must be updated when a layer is completed for a module.
📝 Proposed update
| Layer | Status | Notes | |---|---|---| | Domain | ⚠️ Partial | `FormDefinition`, `FormField` aggregates; all field types and domain events. Missing: `FormSubmission` aggregate (required for F04 form task lifecycle — submission, expiry, deduplication) | | Application | ✅ Done | `CreateFormCommand`, `DeleteFormCommand`, `UpdateFormCommand`, `GetFormsQuery`, `GetFormByIdQuery`, `AddFieldToFormCommand`, `RemoveFieldFromFormCommand`, `ReorderFormFieldsCommand`. Decision: `GetFormsHandler` uses in-memory pagination (accepted MVP trade-off; `Page`/`PageSize` clamped to valid range). | -| Infrastructure | ✅ Done | FormBuilderDbContext, EF Core config (FormDefinition with fields as JSONB via FormFieldConverter), FormRepository (including cross-module IsReferencedByWorkflowAsync JSONB query), 8 integration tests (Testcontainers) | -| API | ⏳ Pending | — | +| Infrastructure | ✅ Done | FormBuilderDbContext, EF Core config (FormDefinition with fields as JSONB via FormFieldConverter), FormWorkflowReference read model with Wolverine handlers (WorkflowPublished/Archived/Unarchived), FormRepository, 8 integration tests (Testcontainers) | +| API | ✅ Done | FormEndpoints with Create/Update/Delete/Get/AddField/RemoveField/ReorderFields; custom JSON converters for field configs; 9 integration tests | | Frontend | ⏳ Pending | — |🤖 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 `@docs/epics/E05-form-builder/README.md` around lines 79 - 85, Update the Implementation Status table to mark the API layer as completed and adjust the Infrastructure note: change the API row from "⏳ Pending" to "✅ Done" since FormEndpoints.cs and FormEndpointTests.cs implement and test the CRUD endpoints; in the Infrastructure row, replace the phrase "cross-module IsReferencedByWorkflowAsync JSONB query" with a description that the cross-module query was replaced by the local FormWorkflowReference read model synchronized via Wolverine handlers (referencing the FormWorkflowReference read model and the Wolverine handlers implemented in Layers 10-11) so the table accurately reflects the current implementation.
🧹 Nitpick comments (5)
tests/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure.Tests/Repositories/ExecutionRepositoryTests.cs (1)
177-177: ⚡ Quick winRemove the WHAT-style inline comment.
Line 177 restates behavior rather than rationale; please remove it (or rewrite with a true WHY if needed).
Proposed change
- // Workflow never published → no row in workflow_active_statuses → falseAs per coding guidelines
**/*.{cs,ts,tsx}: Comments must explain WHY only—never explain WHAT; default to no comments.🤖 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 `@tests/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure.Tests/Repositories/ExecutionRepositoryTests.cs` at line 177, Remove the WHAT-style inline comment "Workflow never published → no row in workflow_active_statuses → false" from ExecutionRepositoryTests (class ExecutionRepositoryTests) — either delete it or replace it with a WHY-style comment that explains the rationale/intent for the test (e.g., why absence of a workflow_active_statuses row should yield false), keeping the comment focused on intent rather than restating behavior.docs/README.md (1)
47-47: ⚡ Quick winConsider clarifying the working directory for diagram generation commands.
The instruction to run
node docs/diagrams/generate-diagrams.mjsdoesn't specify the expected working directory. Adding "from the repository root" would improve clarity for new contributors.📝 Proposed documentation clarification
-All diagrams are Excalidraw (`.excalidraw` source + `.svg` preview). Regenerate with `node docs/diagrams/generate-diagrams.mjs` then `docs/scripts/generate-diagrams.ps1`. +All diagrams are Excalidraw (`.excalidraw` source + `.svg` preview). Regenerate from the repository root with `node docs/diagrams/generate-diagrams.mjs` then `docs/scripts/generate-diagrams.ps1`.🤖 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 `@docs/README.md` at line 47, Update the README sentence about diagram regeneration to specify the expected working directory: change the line that mentions running node docs/diagrams/generate-diagrams.mjs and docs/scripts/generate-diagrams.ps1 to include "from the repository root" (referencing the commands node docs/diagrams/generate-diagrams.mjs and docs/scripts/generate-diagrams.ps1) so contributors know where to run these scripts.docs/scripts/generate-diagrams.ps1 (1)
39-47: ⚡ Quick winConsider using a glob pattern to discover epic diagram directories.
The current implementation hardcodes E01-E06, but E07 (PageBuilder) exists in the repo and future epics may add diagrams. A glob pattern would automatically discover all epic diagram folders without requiring manual updates.
♻️ Proposed fix using glob pattern
-# Collect all .excalidraw files from docs/diagrams/ and docs/epics/*/diagrams/ -$searchPaths = @( - "$docsRoot\diagrams", - "$docsRoot\epics\E01-platform-foundation\diagrams", - "$docsRoot\epics\E02-identity-access\diagrams", - "$docsRoot\epics\E03-data-modeling\diagrams", - "$docsRoot\epics\E04-workflow-builder\diagrams", - "$docsRoot\epics\E05-form-builder\diagrams", - "$docsRoot\epics\E06-workflow-engine\diagrams" -) +# Collect all .excalidraw files from docs/diagrams/ and docs/epics/*/diagrams/ +$searchPaths = @("$docsRoot\diagrams") +$searchPaths += Get-ChildItem -Path "$docsRoot\epics" -Directory | + Where-Object { $_.Name -match '^E\d+-' } | + ForEach-Object { Join-Path $_.FullName "diagrams" } | + Where-Object { Test-Path $_ }🤖 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 `@docs/scripts/generate-diagrams.ps1` around lines 39 - 47, Replace the hardcoded epic entries in $searchPaths with a dynamic discovery: keep "$docsRoot\diagrams" then use Get-ChildItem -Path "$docsRoot\epics" -Directory -Filter "E*-*" (or Where-Object { $_.Name -match '^E\d+' }) to enumerate epic folders, for each build the epic diagrams path (Join-Path $_.FullName 'diagrams'), guard with Test-Path and add existing diagram paths to $searchPaths; update the code around the $searchPaths variable so new epics (e.g., E07) are discovered automatically.src/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Handlers/WorkflowPublishedHandler.cs (1)
22-22: ⚡ Quick winRemove unused variable.
existingFormIdsis constructed but never referenced.♻️ Proposed fix
- // Upsert current references - HashSet<Guid> existingFormIds = [.. existing.Select(r => r.FormId)]; - foreach (Guid formId in `@event.ReferencedFormIds`) + // Upsert current references + foreach (Guid formId in `@event.ReferencedFormIds`)🤖 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 `@src/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Handlers/WorkflowPublishedHandler.cs` at line 22, Remove the unused local variable existingFormIds: delete the HashSet<Guid> existingFormIds = [.. existing.Select(r => r.FormId)]; declaration inside WorkflowPublishedHandler (the variable is never referenced), or if the intent was to use it, replace usages accordingly; locate the construction that references existing and r.FormId and either remove that line or integrate the set into the logic that needs to check existence.src/Axis.Api/Infrastructure/UpdateFieldRequestConverter.cs (1)
16-17: ⚡ Quick winImprove error messages for missing required properties.
Using
GetProperty()directly throwsKeyNotFoundExceptionwith a generic dictionary message when a required field is missing. API consumers would benefit from clearer error messages.Consider validating required fields with
TryGetPropertyand throwingJsonExceptionwith descriptive messages:📋 Proposed improvement for required field validation
- FieldType type = JsonSerializer.Deserialize<FieldType>(root.GetProperty("type").GetRawText(), options); - string label = root.GetProperty("label").GetString()!; + if (!root.TryGetProperty("type", out JsonElement typeEl)) + throw new JsonException("Missing required property 'type'"); + FieldType type = JsonSerializer.Deserialize<FieldType>(typeEl.GetRawText(), options); + + if (!root.TryGetProperty("label", out JsonElement labelEl)) + throw new JsonException("Missing required property 'label'"); + string? label = labelEl.GetString(); + if (label is null) + throw new JsonException("Property 'label' cannot be null"); + string? helpText = root.TryGetProperty("help_text", out JsonElement helpEl) ? helpEl.GetString() : null; - bool isRequired = root.GetProperty("is_required").GetBoolean(); + + if (!root.TryGetProperty("is_required", out JsonElement isReqEl)) + throw new JsonException("Missing required property 'is_required'"); + bool isRequired = isReqEl.GetBoolean();Also applies to: 19-19
🤖 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 `@src/Axis.Api/Infrastructure/UpdateFieldRequestConverter.cs` around lines 16 - 17, Update UpdateFieldRequestConverter to validate required properties using JsonElement.TryGetProperty instead of GetProperty so missing fields produce clear JsonException messages; specifically check for "type" when deserializing FieldType (root.TryGetProperty("type", out var typeProp) then deserialize using typeProp.GetRawText() and options) and for "label" (root.TryGetProperty("label", out var labelProp) then use labelProp.GetString()), and throw new JsonException with a descriptive message like "Missing required property 'type'" or "Missing required property 'label'" when a property is absent; apply the same pattern for the other required property accessed at the same converter (the property around line 19).
🤖 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 `@CLAUDE.md`:
- Around line 337-343: The doc contains two conflicting wireframe-location
rules; standardize to one canonical convention (prefer the new epic-scoped rule)
by updating all occurrences that reference docs/wireframes/*.excalidraw to
instead point to
docs/epics/{E0N-module-name}/wireframes/{screen-slug}.excalidraw and note that
shared layouts remain in docs/wireframes/; specifically edit the "Location"
bullet and any later bullets or callouts that still mention
docs/wireframes/*.excalidraw (including the paragraph that currently suggests
placing wireframes under docs/wireframes and the duplicate at the 454-460 area)
so the file consistently instructs per-epic wireframes, the required kebab-case
naming, and the generate-SVG step scanning both the shared docs/wireframes/ and
all docs/epics/*/wireframes/ directories.
In `@docs/epics/E01-platform-foundation/diagrams/tenant-provisioning.excalidraw`:
- Around line 8-9: Several diagram elements (diag-1, diag-27, diag-28, diag-29,
diag-30) have null geometry values (e.g., "x": null, "y": 12.5) which must be
replaced with concrete numeric coordinates; regenerate the diagram file to
normalize geometry by running the project’s diagram-generation script, then
verify in
docs/epics/E01-platform-foundation/diagrams/tenant-provisioning.excalidraw that
all coordinate and size fields for the listed element IDs (x, y, width, height
where present) contain numeric values (no nulls) and commit the regenerated
file.
In `@docs/epics/E01-platform-foundation/features/F01-tenant-registration.md`:
- Around line 1-3: Replace the plain back-link line below the document title
"F01 — Tenant Registration & Provisioning" with the standardized navigation
callout: remove the existing "[← Back to E01](../README.md)" line and add a
single line after the title reading "> **Navigation**: [← Back to
E01](../README.md)" so the file conforms to the docs/**/*.md guideline for
back-link navigation.
In `@docs/epics/E01-platform-foundation/features/F02-organization-management.md`:
- Around line 1-3: Replace the plain back-link line "[← Back to
E01](../README.md)" immediately under the title "F02 — Organization Management"
with the standardized navigation callout format: a blockquote starting with ">
**Navigation**: [← Back to E01](../README.md)". Ensure the navigation line
appears directly after the title and follows the exact casing and punctuation
shown.
In `@docs/epics/E01-platform-foundation/features/F04-subscription-plans.md`:
- Around line 1-3: Replace the plain back-link line below the title "F04 —
Subscription Plan Management" with the standardized navigation callout format;
locate the existing "[← Back to E01](../README.md)" line and change it to a
single line beginning with "> **Navigation**: " followed by the same link (for
example "> **Navigation**: [← Back to E01](../README.md)"), ensuring the
navigation callout is immediately under the title.
In `@docs/epics/E02-identity-access/diagrams/auth-flow.excalidraw`:
- Around line 8-9: Several Excalidraw elements (diag-1, diag-20, diag-21,
diag-22, diag-23, diag-24, diag-25, diag-26, diag-27) have null geometry fields
(e.g., x: null, width: null) which must be replaced with valid coordinates; open
the diagram in Excalidraw and re-save it to regenerate complete element
properties (or manually fill in the missing geometry for the listed elements),
then run docs/scripts/generate-diagrams.ps1 to regenerate the SVG so the
exported file contains no null coordinate values.
In `@docs/epics/E06-workflow-engine/diagrams/execution-flow.excalidraw`:
- Around line 8-9: Regenerate the Excalidraw diagram to remove null geometry
values by running the provided script docs/scripts/generate-diagrams.ps1, then
commit the updated execution-flow.excalidraw and regenerated SVG preview;
specifically, open
docs/epics/E06-workflow-engine/diagrams/execution-flow.excalidraw, ensure all
geometry fields (x, y, width, height) for elements do not contain null (replace
with numeric defaults if needed), run docs/scripts/generate-diagrams.ps1 to
produce the corrected .excalidraw and SVG, and verify elements previously
flagged (around the snippets showing "x": null / "y": 12.5) no longer contain
null values before committing.
In `@docs/playbooks/patterns.md`:
- Around line 1268-1272: Update the Contents anchor for the renamed section:
change the link target that points to "`#cross-module-read-pattern`" in the
Contents list to match the new header slug "`#cross-module-data-pattern`" (or
whatever the slug generated by the markdown renderer for the header
"Cross-module data pattern" is), so the table-of-contents entry for the
"Cross-module data pattern" section navigates correctly; locate the TOC entry
referencing "cross-module-read-pattern" and replace that anchor with the new
anchor used by the "## Cross-module data pattern" header.
- Around line 1316-1326: The example FormStepAddedHandler.Handle currently
always inserts a new FormWorkflowReference which violates the idempotency rule;
update Handle in FormStepAddedHandler to perform a check-before-insert or upsert
using the db.FormWorkflowReferences set: query for an existing
FormWorkflowReference with matching FormId, WorkflowId and StepId and only Add a
new entity if none exists (or use a database-level upsert/save-or-update if
available), then call SaveChangesAsync(ct); ensure the logic references the
FormStepAdded event properties (evt.FormId, evt.WorkflowId, evt.StepId) and
preserves async/cancellation behavior.
- Line 1345: The sentence is ungrammatical; update the phrase "needs derived
state (e.g., "is the form referenced at all?")" to something like "needs derived
state to be computed (e.g., "is the form referenced at all?")" so the sentence
reads: "If the consuming module needs derived state to be computed (e.g., 'is
the form referenced at all?'), compute it from the local copy at query time — do
not try to sync aggregated state." Replace the original phrasing in the same
sentence in patterns.md.
In `@src/Axis.Api/Infrastructure/AddFormFieldRequestConverter.cs`:
- Around line 18-24: AddFormFieldRequestConverter currently skips calling
FormFieldConfigDeserializer.Deserialize when the "config" property is missing,
allowing invalid FormFieldType values to bypass validation; change the logic to
mirror UpdateFieldRequestConverter by always calling
FormFieldConfigDeserializer.Deserialize(type, configEl?, options) (pass a
default/empty JsonElement or use the TryGetProperty result but still invoke the
deserializer) so the deserializer's switch/validation for FormFieldType always
runs; update the construction in AddFormFieldRequestConverter (the code that
builds AddFormFieldRequest) to use the returned FormFieldConfig from
FormFieldConfigDeserializer.Deserialize even when "config" was absent, ensuring
FormFieldType is validated consistently with UpdateFieldRequestConverter.
In
`@src/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Handlers/WorkflowArchivedHandler.cs`:
- Line 11: Replace the inline fully-qualified type
Domain.ReadModels.FormWorkflowReference used in WorkflowArchivedHandler.cs
(e.g., in the expression context.FormWorkflowReferences and any other
occurrences) by adding a using directive for Axis.FormBuilder.Domain.ReadModels
at the top of the file and then change the type usages to the short name
FormWorkflowReference; ensure all references (including the variable declaration
List<Domain.ReadModels.FormWorkflowReference> refs and any other
Domain.ReadModels.* occurrences in this handler) are updated accordingly.
In
`@src/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Handlers/WorkflowPublishedHandler.cs`:
- Around line 12-14: The query in WorkflowPublishedHandler that loads existing
FormWorkflowReference entries (context.FormWorkflowReferences.Where(r =>
r.WorkflowId == `@event.WorkflowId`)) must also filter by OrganizationId to avoid
cross-tenant mixing; update the LINQ to include r.OrganizationId ==
`@event.OrganizationId` (and optionally guard that `@event.OrganizationId` is
present) so only references for the same organization are returned when
processing FormWorkflowReference records for the given `@event.WorkflowId`.
In
`@src/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Handlers/WorkflowUnarchivedHandler.cs`:
- Around line 11-13: Replace fully qualified type usage
Domain.ReadModels.FormWorkflowReference with the short name by adding a using
for the Domain.ReadModels namespace at the top of WorkflowUnarchivedHandler.cs,
then update the two occurrences (the List declaration using
FormWorkflowReference and the LINQ/ToListAsync call referencing
FormWorkflowReference) to use FormWorkflowReference instead of the fully
qualified name; ensure there are no other references relying on the fully
qualified path and remove any redundant using if needed.
- Around line 11-13: The query in WorkflowUnarchivedHandler that loads refs from
FormWorkflowReferences only filters by `@event.WorkflowId` and IsActive, risking
cross-organization reactivation; update the LINQ expression used to populate
refs to also filter by r.OrganizationId == `@event.OrganizationId` (i.e., add &&
r.OrganizationId == `@event.OrganizationId`) so the retrieved list only contains
references for the same organization as the event before calling any
reactivation logic.
In
`@src/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure/Handlers/WorkflowPublishedHandler.cs`:
- Around line 12-22: The current read-then-insert flow in
WorkflowPublishedHandler (uses context.WorkflowActiveStatuses,
WorkflowActiveStatus.Activated, existing.Reactivate and await
context.SaveChangesAsync) can race and cause a PK violation; make it idempotent
by wrapping the add+SaveChangesAsync sequence in a try/catch that catches the EF
DbUpdateException (or database-specific duplicate-key exception), detect a
duplicate-key/unique-constraint error, and swallow or reconcile it (e.g., reload
the existing WorkflowActiveStatus and call existing.Reactivate if needed)
instead of letting the exception bubble; ensure the code still calls
existing.Reactivate when an existing row is found and only treats duplicate-key
errors as a benign, expected outcome.
In `@tests/Api/Axis.Api.Tests/FormBuilder/FormEndpointTests.cs`:
- Around line 19-20: Replace every use of implicit var in FormEndpointTests.cs
with explicit CLR types per the guideline; e.g., change "var resp = await
fixture.Client.GetAsync(...)" to "HttpResponseMessage resp = await
fixture.Client.GetAsync(...)", replace "var client" to "HttpClient client", "var
json" to "JsonElement" or the appropriate System.Text.Json type, and similarly
annotate results of PostAsJsonAsync, ReadFromJsonAsync, GetAsync, and any other
declarations; ensure constructors use target-typed new only where allowed and
keep all existing using directives to resolve types referenced in methods like
GetAsync, PostAsJsonAsync, ReadFromJsonAsync, and any helper methods in the test
class.
In `@tests/Api/Axis.Api.Tests/Helpers/ApiTestFixture.cs`:
- Around line 157-167: Replace the implicit 'var' declarations for the DbContext
options with explicit types: use DbContextOptions<FormBuilderDbContext> for
fbOptions and DbContextOptions<WorkflowEngineDbContext> for weOptions (and
similarly for the other two option variables referenced in this method), keeping
the subsequent DbContext instantiations (fbCtx and weCtx) as target-typed new
expressions per guideline; update the four declarations that currently use 'var'
(fbOptions, the corresponding first DB options, weOptions, and the corresponding
second DB options) to their explicit generic DbContextOptions<T> types.
---
Outside diff comments:
In `@docs/epics/E05-form-builder/README.md`:
- Around line 79-85: Update the Implementation Status table to mark the API
layer as completed and adjust the Infrastructure note: change the API row from
"⏳ Pending" to "✅ Done" since FormEndpoints.cs and FormEndpointTests.cs
implement and test the CRUD endpoints; in the Infrastructure row, replace the
phrase "cross-module IsReferencedByWorkflowAsync JSONB query" with a description
that the cross-module query was replaced by the local FormWorkflowReference read
model synchronized via Wolverine handlers (referencing the FormWorkflowReference
read model and the Wolverine handlers implemented in Layers 10-11) so the table
accurately reflects the current implementation.
---
Nitpick comments:
In `@docs/README.md`:
- Line 47: Update the README sentence about diagram regeneration to specify the
expected working directory: change the line that mentions running node
docs/diagrams/generate-diagrams.mjs and docs/scripts/generate-diagrams.ps1 to
include "from the repository root" (referencing the commands node
docs/diagrams/generate-diagrams.mjs and docs/scripts/generate-diagrams.ps1) so
contributors know where to run these scripts.
In `@docs/scripts/generate-diagrams.ps1`:
- Around line 39-47: Replace the hardcoded epic entries in $searchPaths with a
dynamic discovery: keep "$docsRoot\diagrams" then use Get-ChildItem -Path
"$docsRoot\epics" -Directory -Filter "E*-*" (or Where-Object { $_.Name -match
'^E\d+' }) to enumerate epic folders, for each build the epic diagrams path
(Join-Path $_.FullName 'diagrams'), guard with Test-Path and add existing
diagram paths to $searchPaths; update the code around the $searchPaths variable
so new epics (e.g., E07) are discovered automatically.
In `@src/Axis.Api/Infrastructure/UpdateFieldRequestConverter.cs`:
- Around line 16-17: Update UpdateFieldRequestConverter to validate required
properties using JsonElement.TryGetProperty instead of GetProperty so missing
fields produce clear JsonException messages; specifically check for "type" when
deserializing FieldType (root.TryGetProperty("type", out var typeProp) then
deserialize using typeProp.GetRawText() and options) and for "label"
(root.TryGetProperty("label", out var labelProp) then use
labelProp.GetString()), and throw new JsonException with a descriptive message
like "Missing required property 'type'" or "Missing required property 'label'"
when a property is absent; apply the same pattern for the other required
property accessed at the same converter (the property around line 19).
In
`@src/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Handlers/WorkflowPublishedHandler.cs`:
- Line 22: Remove the unused local variable existingFormIds: delete the
HashSet<Guid> existingFormIds = [.. existing.Select(r => r.FormId)]; declaration
inside WorkflowPublishedHandler (the variable is never referenced), or if the
intent was to use it, replace usages accordingly; locate the construction that
references existing and r.FormId and either remove that line or integrate the
set into the logic that needs to check existence.
In
`@tests/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure.Tests/Repositories/ExecutionRepositoryTests.cs`:
- Line 177: Remove the WHAT-style inline comment "Workflow never published → no
row in workflow_active_statuses → false" from ExecutionRepositoryTests (class
ExecutionRepositoryTests) — either delete it or replace it with a WHY-style
comment that explains the rationale/intent for the test (e.g., why absence of a
workflow_active_statuses row should yield false), keeping the comment focused on
intent rather than restating behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2903b83e-1d01-4537-93c4-d26303d8c8b1
⛔ Files ignored due to path filters (44)
docs/diagrams/container-diagram.pngis excluded by!**/*.pngdocs/diagrams/container.svgis excluded by!**/*.svgdocs/diagrams/module-overview.pngis excluded by!**/*.pngdocs/diagrams/module-overview.svgis excluded by!**/*.svgdocs/diagrams/system-context.pngis excluded by!**/*.pngdocs/diagrams/system-context.svgis excluded by!**/*.svgdocs/epics/E01-platform-foundation/diagrams/tenant-provisioning.pngis excluded by!**/*.pngdocs/epics/E01-platform-foundation/diagrams/tenant-provisioning.svgis excluded by!**/*.svgdocs/epics/E01-platform-foundation/wireframes/email-confirmation.svgis excluded by!**/*.svgdocs/epics/E01-platform-foundation/wireframes/pricing.svgis excluded by!**/*.svgdocs/epics/E01-platform-foundation/wireframes/register-org.svgis excluded by!**/*.svgdocs/epics/E01-platform-foundation/wireframes/settings-org-delete-modal.svgis excluded by!**/*.svgdocs/epics/E01-platform-foundation/wireframes/settings-org.svgis excluded by!**/*.svgdocs/epics/E01-platform-foundation/wireframes/verify-email.svgis excluded by!**/*.svgdocs/epics/E01-platform-foundation/wireframes/workspace-provisioning.svgis excluded by!**/*.svgdocs/epics/E02-identity-access/diagrams/auth-flow.pngis excluded by!**/*.pngdocs/epics/E02-identity-access/diagrams/auth-flow.svgis excluded by!**/*.svgdocs/epics/E02-identity-access/wireframes/accept-invitation.svgis excluded by!**/*.svgdocs/epics/E02-identity-access/wireframes/change-password.svgis excluded by!**/*.svgdocs/epics/E02-identity-access/wireframes/forgot-password.svgis excluded by!**/*.svgdocs/epics/E02-identity-access/wireframes/login.svgis excluded by!**/*.svgdocs/epics/E02-identity-access/wireframes/register.svgis excluded by!**/*.svgdocs/epics/E02-identity-access/wireframes/settings-roles.svgis excluded by!**/*.svgdocs/epics/E02-identity-access/wireframes/settings-security.svgis excluded by!**/*.svgdocs/epics/E02-identity-access/wireframes/settings-users.svgis excluded by!**/*.svgdocs/epics/E03-data-modeling/diagrams/data-model.pngis excluded by!**/*.pngdocs/epics/E03-data-modeling/diagrams/data-model.svgis excluded by!**/*.svgdocs/epics/E03-data-modeling/wireframes/data-classes.svgis excluded by!**/*.svgdocs/epics/E03-data-modeling/wireframes/data-models.svgis excluded by!**/*.svgdocs/epics/E03-data-modeling/wireframes/records.svgis excluded by!**/*.svgdocs/epics/E04-workflow-builder/diagrams/workflow-model.pngis excluded by!**/*.pngdocs/epics/E04-workflow-builder/diagrams/workflow-model.svgis excluded by!**/*.svgdocs/epics/E04-workflow-builder/wireframes/workflow-editor.svgis excluded by!**/*.svgdocs/epics/E04-workflow-builder/wireframes/workflows.svgis excluded by!**/*.svgdocs/epics/E05-form-builder/diagrams/form-model.pngis excluded by!**/*.pngdocs/epics/E05-form-builder/diagrams/form-model.svgis excluded by!**/*.svgdocs/epics/E05-form-builder/wireframes/form-editor.svgis excluded by!**/*.svgdocs/epics/E05-form-builder/wireframes/form-submission.svgis excluded by!**/*.svgdocs/epics/E05-form-builder/wireframes/forms.svgis excluded by!**/*.svgdocs/epics/E06-workflow-engine/diagrams/execution-flow.pngis excluded by!**/*.pngdocs/epics/E06-workflow-engine/diagrams/execution-flow.svgis excluded by!**/*.svgdocs/epics/E06-workflow-engine/wireframes/execution-detail.svgis excluded by!**/*.svgdocs/epics/E06-workflow-engine/wireframes/executions.svgis excluded by!**/*.svgdocs/wireframes/app-shell.svgis excluded by!**/*.svg
📒 Files selected for processing (143)
CLAUDE.mddocs/ARCHITECTURE.mddocs/PROGRESS.mddocs/README.mddocs/diagrams/container-diagram.pumldocs/diagrams/container.excalidrawdocs/diagrams/generate-diagrams.mjsdocs/diagrams/module-overview.excalidrawdocs/diagrams/module-overview.pumldocs/diagrams/system-context.excalidrawdocs/diagrams/system-context.pumldocs/epics/E01-platform-foundation/README.mddocs/epics/E01-platform-foundation/diagrams/tenant-provisioning.excalidrawdocs/epics/E01-platform-foundation/diagrams/tenant-provisioning.pumldocs/epics/E01-platform-foundation/features/F01-tenant-registration.mddocs/epics/E01-platform-foundation/features/F02-organization-management.mddocs/epics/E01-platform-foundation/features/F04-subscription-plans.mddocs/epics/E01-platform-foundation/wireframes/email-confirmation.excalidrawdocs/epics/E01-platform-foundation/wireframes/pricing.excalidrawdocs/epics/E01-platform-foundation/wireframes/register-org.excalidrawdocs/epics/E01-platform-foundation/wireframes/settings-org-delete-modal.excalidrawdocs/epics/E01-platform-foundation/wireframes/settings-org.excalidrawdocs/epics/E01-platform-foundation/wireframes/verify-email.excalidrawdocs/epics/E01-platform-foundation/wireframes/workspace-provisioning.excalidrawdocs/epics/E02-identity-access/README.mddocs/epics/E02-identity-access/diagrams/auth-flow.excalidrawdocs/epics/E02-identity-access/diagrams/auth-flow.pumldocs/epics/E02-identity-access/features/F01-authentication.mddocs/epics/E02-identity-access/features/F02-user-management.mddocs/epics/E02-identity-access/features/F03-role-management.mddocs/epics/E02-identity-access/features/F04-permissions.mddocs/epics/E02-identity-access/features/F05-password-security.mddocs/epics/E02-identity-access/wireframes/accept-invitation.excalidrawdocs/epics/E02-identity-access/wireframes/change-password.excalidrawdocs/epics/E02-identity-access/wireframes/forgot-password.excalidrawdocs/epics/E02-identity-access/wireframes/login.excalidrawdocs/epics/E02-identity-access/wireframes/register.excalidrawdocs/epics/E02-identity-access/wireframes/settings-roles.excalidrawdocs/epics/E02-identity-access/wireframes/settings-security.excalidrawdocs/epics/E02-identity-access/wireframes/settings-users.excalidrawdocs/epics/E03-data-modeling/README.mddocs/epics/E03-data-modeling/diagrams/data-model.excalidrawdocs/epics/E03-data-modeling/diagrams/data-model.pumldocs/epics/E03-data-modeling/features/F01-model-definition.mddocs/epics/E03-data-modeling/features/F02-field-types.mddocs/epics/E03-data-modeling/features/F03-data-classes.mddocs/epics/E03-data-modeling/features/F04-data-records.mddocs/epics/E03-data-modeling/wireframes/data-classes.excalidrawdocs/epics/E03-data-modeling/wireframes/data-models.excalidrawdocs/epics/E03-data-modeling/wireframes/records.excalidrawdocs/epics/E04-workflow-builder/README.mddocs/epics/E04-workflow-builder/diagrams/workflow-model.excalidrawdocs/epics/E04-workflow-builder/diagrams/workflow-model.pumldocs/epics/E04-workflow-builder/features/F01-workflow-definition.mddocs/epics/E04-workflow-builder/features/F02-visual-canvas.mddocs/epics/E04-workflow-builder/features/F03-step-types.mddocs/epics/E04-workflow-builder/features/F04-triggers.mddocs/epics/E04-workflow-builder/features/F05-branching.mddocs/epics/E04-workflow-builder/features/F06-parallel-execution.mddocs/epics/E04-workflow-builder/features/F07-import-export.mddocs/epics/E04-workflow-builder/wireframes/workflow-editor.excalidrawdocs/epics/E04-workflow-builder/wireframes/workflows.excalidrawdocs/epics/E05-form-builder/README.mddocs/epics/E05-form-builder/diagrams/form-model.excalidrawdocs/epics/E05-form-builder/diagrams/form-model.pumldocs/epics/E05-form-builder/features/F01-form-definition.mddocs/epics/E05-form-builder/features/F02-form-fields.mddocs/epics/E05-form-builder/features/F03-workflow-integration.mddocs/epics/E05-form-builder/features/F04-form-submission.mddocs/epics/E05-form-builder/wireframes/form-editor.excalidrawdocs/epics/E05-form-builder/wireframes/form-submission.excalidrawdocs/epics/E05-form-builder/wireframes/forms.excalidrawdocs/epics/E06-workflow-engine/README.mddocs/epics/E06-workflow-engine/diagrams/execution-flow.excalidrawdocs/epics/E06-workflow-engine/diagrams/execution-flow.pumldocs/epics/E06-workflow-engine/features/F01-execution-management.mddocs/epics/E06-workflow-engine/features/F02-step-handlers.mddocs/epics/E06-workflow-engine/features/F03-error-handling.mddocs/epics/E06-workflow-engine/features/F04-execution-history.mddocs/epics/E06-workflow-engine/features/F05-manual-retry.mddocs/epics/E06-workflow-engine/wireframes/execution-detail.excalidrawdocs/epics/E06-workflow-engine/wireframes/executions.excalidrawdocs/epics/E07-page-builder/README.mddocs/playbooks/patterns.mddocs/playbooks/process.mddocs/playbooks/wireframes.mddocs/scripts/generate-diagrams.ps1docs/scripts/generate-wireframes.ps1docs/wireframes/app-shell.excalidrawdocs/wireframes/generate-screens.mjssrc/Axis.Api/Endpoints/AddDataClassFieldRequest.cssrc/Axis.Api/Endpoints/AddFieldRequest.cssrc/Axis.Api/Endpoints/AddFormFieldRequest.cssrc/Axis.Api/Endpoints/CreateFormRequest.cssrc/Axis.Api/Endpoints/DataClassEndpoints.cssrc/Axis.Api/Endpoints/FieldConfigHelper.cssrc/Axis.Api/Endpoints/FormEndpoints.cssrc/Axis.Api/Endpoints/ModelEndpoints.cssrc/Axis.Api/Endpoints/ReorderFormFieldsRequest.cssrc/Axis.Api/Endpoints/UpdateFieldRequest.cssrc/Axis.Api/Endpoints/UpdateFormRequest.cssrc/Axis.Api/Infrastructure/AddDataClassFieldRequestConverter.cssrc/Axis.Api/Infrastructure/AddFieldRequestConverter.cssrc/Axis.Api/Infrastructure/AddFormFieldRequestConverter.cssrc/Axis.Api/Infrastructure/FieldConfigDeserializer.cssrc/Axis.Api/Infrastructure/FormFieldConfigDeserializer.cssrc/Axis.Api/Infrastructure/UpdateFieldRequestConverter.cssrc/Axis.Api/Program.cssrc/Modules/FormBuilder/Axis.FormBuilder.Application/Commands/DeleteForm/DeleteFormHandler.cssrc/Modules/FormBuilder/Axis.FormBuilder.Domain/ReadModels/FormWorkflowReference.cssrc/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/AssemblyInfo.cssrc/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Axis.FormBuilder.Infrastructure.csprojsrc/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Handlers/WorkflowArchivedHandler.cssrc/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Handlers/WorkflowPublishedHandler.cssrc/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Handlers/WorkflowUnarchivedHandler.cssrc/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Migrations/20260519044541_AddFormWorkflowReferences.Designer.cssrc/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Migrations/20260519044541_AddFormWorkflowReferences.cssrc/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Migrations/FormBuilderDbContextModelSnapshot.cssrc/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Persistence/Configurations/FormDefinitionConfiguration.cssrc/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Persistence/Configurations/FormWorkflowReferenceConfiguration.cssrc/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Persistence/FormBuilderDbContext.cssrc/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Repositories/FormRepository.cssrc/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Domain/Aggregates/WorkflowDefinition.cssrc/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Domain/Entities/WorkflowStep.cssrc/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Domain/Events/WorkflowPublished.cssrc/Modules/WorkflowBuilder/Axis.WorkflowBuilder.Domain/Events/WorkflowUnarchived.cssrc/Modules/WorkflowEngine/Axis.WorkflowEngine.Domain/Aggregates/WorkflowActiveStatus.cssrc/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure/AssemblyInfo.cssrc/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure/Axis.WorkflowEngine.Infrastructure.csprojsrc/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure/Handlers/WorkflowArchivedHandler.cssrc/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure/Handlers/WorkflowPublishedHandler.cssrc/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure/Handlers/WorkflowUnarchivedHandler.cssrc/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure/Migrations/20260519044503_AddWorkflowActiveStatuses.Designer.cssrc/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure/Migrations/20260519044503_AddWorkflowActiveStatuses.cssrc/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure/Migrations/WorkflowEngineDbContextModelSnapshot.cssrc/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure/Persistence/Configurations/WorkflowActiveStatusConfiguration.cssrc/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure/Persistence/WorkflowEngineDbContext.cssrc/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure/Services/WorkflowDefinitionReader.cstests/Api/Axis.Api.Tests/FormBuilder/FormEndpointTests.cstests/Api/Axis.Api.Tests/Helpers/ApiTestFixture.cstests/Api/Axis.Api.Tests/Helpers/NullFormBuilderUnitOfWork.cstests/Modules/FormBuilder/Axis.FormBuilder.Infrastructure.Tests/Repositories/FormRepositoryTests.cstests/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure.Tests/Repositories/ExecutionRepositoryTests.cs
💤 Files with no reviewable changes (10)
- src/Axis.Api/Endpoints/FieldConfigHelper.cs
- docs/diagrams/system-context.puml
- docs/epics/E04-workflow-builder/diagrams/workflow-model.puml
- docs/epics/E05-form-builder/diagrams/form-model.puml
- docs/diagrams/module-overview.puml
- docs/epics/E01-platform-foundation/diagrams/tenant-provisioning.puml
- docs/epics/E03-data-modeling/diagrams/data-model.puml
- docs/diagrams/container-diagram.puml
- docs/epics/E06-workflow-engine/diagrams/execution-flow.puml
- docs/epics/E02-identity-access/diagrams/auth-flow.puml
Comment on lines
+1
to
3
| # F01 — Tenant Registration & Provisioning | ||
|
|
||
| [← Back to E01](../README.md) |
There was a problem hiding this comment.
Use the standardized navigation callout format under the title.
Please replace the plain back-link line with the required > **Navigation**: ... style for consistency across docs.
As per coding guidelines: "docs/**/*.md: Add back-link navigation after every doc title: > Navigation: ← parent or to docs/README.md for sub-folders".
🧰 Tools
🪛 LanguageTool
[grammar] ~1-~1: Ensure spelling is correct
Context: # F01 — Tenant Registration & Provisioning ← Back to E01 > **Wiref...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@docs/epics/E01-platform-foundation/features/F01-tenant-registration.md`
around lines 1 - 3, Replace the plain back-link line below the document title
"F01 — Tenant Registration & Provisioning" with the standardized navigation
callout: remove the existing "[← Back to E01](../README.md)" line and add a
single line after the title reading "> **Navigation**: [← Back to
E01](../README.md)" so the file conforms to the docs/**/*.md guideline for
back-link navigation.
Comment on lines
+1
to
3
| # F02 — Organization Management | ||
|
|
||
| [← Back to E01](../README.md) |
There was a problem hiding this comment.
Use the standardized navigation callout format under the title.
Please replace the plain back-link line with the required > **Navigation**: ... style.
As per coding guidelines: "docs/**/*.md: Add back-link navigation after every doc title: > Navigation: ← parent or to docs/README.md for sub-folders".
🤖 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 `@docs/epics/E01-platform-foundation/features/F02-organization-management.md`
around lines 1 - 3, Replace the plain back-link line "[← Back to
E01](../README.md)" immediately under the title "F02 — Organization Management"
with the standardized navigation callout format: a blockquote starting with ">
**Navigation**: [← Back to E01](../README.md)". Ensure the navigation line
appears directly after the title and follows the exact casing and punctuation
shown.
Comment on lines
+1
to
3
| # F04 — Subscription Plan Management | ||
|
|
||
| [← Back to E01](../README.md) |
There was a problem hiding this comment.
Use the standardized navigation callout format under the title.
Please replace the plain back-link line with the required > **Navigation**: ... style.
As per coding guidelines: "docs/**/*.md: Add back-link navigation after every doc title: > Navigation: ← parent or to docs/README.md for sub-folders".
🧰 Tools
🪛 LanguageTool
[grammar] ~1-~1: Ensure spelling is correct
Context: # F04 — Subscription Plan Management ← Back to E01 > **Wiref...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@docs/epics/E01-platform-foundation/features/F04-subscription-plans.md` around
lines 1 - 3, Replace the plain back-link line below the title "F04 —
Subscription Plan Management" with the standardized navigation callout format;
locate the existing "[← Back to E01](../README.md)" line and change it to a
single line beginning with "> **Navigation**: " followed by the same link (for
example "> **Navigation**: [← Back to E01](../README.md)"), ensuring the
navigation callout is immediately under the title.
Comment on lines
+12
to
+22
| WorkflowActiveStatus? existing = await context.WorkflowActiveStatuses | ||
| .FirstOrDefaultAsync(w => w.WorkflowId == @event.WorkflowId, ct); | ||
|
|
||
| if (existing is null) | ||
| context.WorkflowActiveStatuses.Add( | ||
| WorkflowActiveStatus.Activated(@event.WorkflowId, @event.OrganizationId)); | ||
| else | ||
| existing.Reactivate(); | ||
|
|
||
| await context.SaveChangesAsync(ct); | ||
| } |
There was a problem hiding this comment.
Make publish handler idempotent under concurrent/duplicate event delivery.
Line 12–22 uses a read-then-insert flow that can race: two deliveries for the same workflow can both see no row and one SaveChangesAsync will fail on the PK. This should be handled as an expected path for event consumers.
Suggested hardening
public async Task Handle(WorkflowPublished `@event`, CancellationToken ct)
{
- WorkflowActiveStatus? existing = await context.WorkflowActiveStatuses
- .FirstOrDefaultAsync(w => w.WorkflowId == `@event.WorkflowId`, ct);
-
- if (existing is null)
- context.WorkflowActiveStatuses.Add(
- WorkflowActiveStatus.Activated(`@event.WorkflowId`, `@event.OrganizationId`));
- else
- existing.Reactivate();
-
- await context.SaveChangesAsync(ct);
+ WorkflowActiveStatus? existing = await context.WorkflowActiveStatuses
+ .FirstOrDefaultAsync(w => w.WorkflowId == `@event.WorkflowId`, ct);
+
+ if (existing is null)
+ {
+ context.WorkflowActiveStatuses.Add(
+ WorkflowActiveStatus.Activated(`@event.WorkflowId`, `@event.OrganizationId`));
+ }
+ else
+ {
+ existing.Reactivate();
+ }
+
+ try
+ {
+ await context.SaveChangesAsync(ct);
+ }
+ catch (DbUpdateException)
+ {
+ // Duplicate insert race: re-read and ensure active state.
+ WorkflowActiveStatus? current = await context.WorkflowActiveStatuses
+ .FirstOrDefaultAsync(w => w.WorkflowId == `@event.WorkflowId`, ct);
+
+ if (current is not null && !current.IsActive)
+ {
+ current.Reactivate();
+ await context.SaveChangesAsync(ct);
+ }
+ }
}🤖 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
`@src/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure/Handlers/WorkflowPublishedHandler.cs`
around lines 12 - 22, The current read-then-insert flow in
WorkflowPublishedHandler (uses context.WorkflowActiveStatuses,
WorkflowActiveStatus.Activated, existing.Reactivate and await
context.SaveChangesAsync) can race and cause a PK violation; make it idempotent
by wrapping the add+SaveChangesAsync sequence in a try/catch that catches the EF
DbUpdateException (or database-specific duplicate-key exception), detect a
duplicate-key/unique-constraint error, and swallow or reconcile it (e.g., reload
the existing WorkflowActiveStatus and call existing.Reactivate if needed)
instead of letting the exception bubble; ensure the code still calls
existing.Reactivate when an existing row is found and only treats duplicate-key
errors as a benign, expected outcome.
Documents approved spec/diagram updates that have not yet been applied to generate-diagrams.mjs, so future sessions can pick them up: - E03 F03: DataClassField entity pending removal from data-model diagram - E03 F04: Record → DataRecord rename pending (C# keyword conflict) - E04 F03: StepType missing Start/End values - E04 F04: TriggerConfig → value object (no id, 1-to-many) - E04 F05: Transition → value object (fromStepId, toStepId, condition only) - E04 F06: ParallelGroup/JoinType to be marked planned (Phase 2) - E05 F04: FormTaskStatus.Completed → Submitted Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 `@docs/epics/E03-data-modeling/features/F04-data-records.md`:
- Line 3: The feature file F04-data-records.md currently links the wireframe at
docs/epics/E03-data-modeling/wireframes/records.excalidraw; update this to the
canonical location docs/wireframes/records.excalidraw (and corresponding
docs/wireframes/records.svg) and regenerate the SVG using
docs/scripts/generate-wireframes.ps1; ensure the "**Wireframe**" callout
(pointing to docs/wireframes/records.excalidraw and its SVG) is placed before
the first user story in F04-data-records.md to match the
docs/epics/*/features/*.md guideline.
In `@docs/epics/E05-form-builder/features/F04-form-submission.md`:
- Line 3: Update the Wireframe link in the feature file
docs/epics/E05-form-builder/features/F04-form-submission.md to point to the
global location docs/wireframes/form-submission.excalidraw and the preview to
docs/wireframes/form-submission.svg (replace the module-local ../wireframes/...
path), ensure the "**Wireframe**" callout remains placed before the first user
story in this feature file, and run docs/scripts/generate-wireframes.ps1 to
produce the SVG alongside the .excalidraw file.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: de5e5bdf-46c6-4322-b53d-8b0dd17b0e37
📒 Files selected for processing (7)
docs/epics/E03-data-modeling/features/F03-data-classes.mddocs/epics/E03-data-modeling/features/F04-data-records.mddocs/epics/E04-workflow-builder/features/F03-step-types.mddocs/epics/E04-workflow-builder/features/F04-triggers.mddocs/epics/E04-workflow-builder/features/F05-branching.mddocs/epics/E04-workflow-builder/features/F06-parallel-execution.mddocs/epics/E05-form-builder/features/F04-form-submission.md
✅ Files skipped from review due to trivial changes (4)
- docs/epics/E03-data-modeling/features/F03-data-classes.md
- docs/epics/E04-workflow-builder/features/F04-triggers.md
- docs/epics/E04-workflow-builder/features/F06-parallel-execution.md
- docs/epics/E04-workflow-builder/features/F05-branching.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/epics/E04-workflow-builder/features/F03-step-types.md
| # F04 — Data Record CRUD | ||
|
|
||
| > **Wireframe**: [docs/wireframes/E03-data-modeling/records.excalidraw](../../../wireframes/E03-data-modeling/records.excalidraw) · [preview](../../../wireframes/E03-data-modeling/records.svg) | ||
| > **Wireframe**: [docs/epics/E03-data-modeling/wireframes/records.excalidraw](../wireframes/records.excalidraw) · [preview](../wireframes/records.svg) |
There was a problem hiding this comment.
Use the canonical wireframe location required by docs guidelines.
Line 3 links the wireframe under docs/epics/.../wireframes, but this path pattern does not match the required docs/wireframes/{screen-slug}.excalidraw convention for feature files.
As per coding guidelines, "docs/epics/*/features/*.md: Every feature must have wireframe at docs/wireframes/{screen-slug}.excalidraw with SVG generated via docs/scripts/generate-wireframes.ps1 alongside it, and > Wireframe callout added to feature file before first US".
🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: Ensure spelling is correct
Context: ...w](../wireframes/records.excalidraw) · preview ← Back to E03 --- ## D...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@docs/epics/E03-data-modeling/features/F04-data-records.md` at line 3, The
feature file F04-data-records.md currently links the wireframe at
docs/epics/E03-data-modeling/wireframes/records.excalidraw; update this to the
canonical location docs/wireframes/records.excalidraw (and corresponding
docs/wireframes/records.svg) and regenerate the SVG using
docs/scripts/generate-wireframes.ps1; ensure the "**Wireframe**" callout
(pointing to docs/wireframes/records.excalidraw and its SVG) is placed before
the first user story in F04-data-records.md to match the
docs/epics/*/features/*.md guideline.
| # F04 — Form Submission Handling | ||
|
|
||
| > **Wireframe**: [docs/wireframes/E05-form-builder/form-submission.excalidraw](../../../wireframes/E05-form-builder/form-submission.excalidraw) · [preview](../../../wireframes/E05-form-builder/form-submission.svg) | ||
| > **Wireframe**: [docs/epics/E05-form-builder/wireframes/form-submission.excalidraw](../wireframes/form-submission.excalidraw) · [preview](../wireframes/form-submission.svg) |
There was a problem hiding this comment.
Align wireframe path with the required global docs location.
Line 3 references a module-local wireframe path; the feature-file guideline requires wireframes under docs/wireframes/{screen-slug}.excalidraw.
As per coding guidelines, "docs/epics/*/features/*.md: Every feature must have wireframe at docs/wireframes/{screen-slug}.excalidraw with SVG generated via docs/scripts/generate-wireframes.ps1 alongside it, and > Wireframe callout added to feature file before first US".
🤖 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 `@docs/epics/E05-form-builder/features/F04-form-submission.md` at line 3,
Update the Wireframe link in the feature file
docs/epics/E05-form-builder/features/F04-form-submission.md to point to the
global location docs/wireframes/form-submission.excalidraw and the preview to
docs/wireframes/form-submission.svg (replace the module-local ../wireframes/...
path), ensure the "**Wireframe**" callout remains placed before the first user
story in this feature file, and run docs/scripts/generate-wireframes.ps1 to
produce the SVG alongside the .excalidraw file.
- Add OrganizationId filter to WorkflowArchivedHandler, WorkflowUnarchivedHandler, and FormBuilder's WorkflowPublishedHandler for proper tenant isolation - Add using directive and remove inline Domain.ReadModels type references in WorkflowArchivedHandler and WorkflowUnarchivedHandler - Wrap SaveChangesAsync in try-catch DbUpdateException in WorkflowEngine's WorkflowPublishedHandler to handle concurrent duplicate event delivery - Fix AddFormFieldRequestConverter to always call Deserialize (validates field type even when config is absent), matching UpdateFieldRequestConverter behavior - Replace var with explicit types in ApiTestFixture and FormEndpointTests - Fix seqBuild() null coordinate bug in generate-diagrams.mjs (CANVAS_W was computed before participant cx values were set in the ps map) - Regenerate all diagram .excalidraw and .svg files with corrected coordinates - Fix broken anchor in patterns.md and make FormStepAddedHandler example idempotent Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/playbooks/patterns.md (1)
1350-1350:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the grammatical construction.
The phrase "needs derived state computed" is nonstandard.
📝 Suggested wording fix
-- If the consuming module needs derived state computed (e.g., "is the form referenced at all?"), compute it from the local copy at query time — do not try to sync aggregated state. +- If the consuming module needs to compute derived state (e.g., "is the form referenced at all?"), compute it from the local copy at query time — do not try to sync aggregated state.Alternative wording:
-- If the consuming module needs derived state computed (e.g., "is the form referenced at all?"), compute it from the local copy at query time — do not try to sync aggregated state. +- If the consuming module needs derived state to be computed (e.g., "is the form referenced at all?"), compute it from the local copy at query time — do not try to sync aggregated state.🤖 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 `@docs/playbooks/patterns.md` at line 1350, Replace the nonstandard phrase "needs derived state computed" in the sentence "If the consuming module needs derived state computed (e.g., "is the form referenced at all?"), compute it from the local copy at query time — do not try to sync aggregated state." with a grammatically correct construction such as "needs derived state to be computed" or "needs derived state computed at query time" so the sentence reads e.g. "If the consuming module needs derived state to be computed (e.g., 'is the form referenced at all?'), compute it from the local copy at query time — do not try to sync aggregated state." Ensure quotes and punctuation remain intact.
🧹 Nitpick comments (1)
docs/playbooks/patterns.md (1)
1356-1356: 💤 Low valueCorrect the code block language identifier.
The code block is marked as
powershellbut contains bash/grep syntax. For consistency with other grep commands in this document (lines 1429, 1456) and correctness, usebashinstead.♻️ Suggested fix
-```powershell +```bash grep -rn "SqlQueryRaw\|ExecuteSqlRaw\|FromSqlRaw\|ExecuteSqlInterpolated" src/Modules/ --include="*.cs"</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/playbooks/patterns.mdat line 1356, Change the fenced code block
language from "powershell" to "bash" for the block that contains the grep
command (grep -rn
"SqlQueryRaw|ExecuteSqlRaw|FromSqlRaw|ExecuteSqlInterpolated" src/Modules/
--include="*.cs") so the syntax highlighting matches the bash/grep content and
aligns with other grep examples in the document.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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@docs/playbooks/patterns.md:
- Line 1316: The document is inconsistent about where event handlers live
(example places FormStepAddedHandler in FormBuilder.Infrastructure/Handlers
while earlier rule says handlers belong in the Application layer); pick a
consistent convention and update the text and examples: either (A) state an
explicit distinction that "data-synchronization or local-copy/table update
handlers (e.g., cross-module sync handlers like FormStepAddedHandler) belong in
Infrastructure while business-logic domain event handlers belong in Application"
and update the rule at the earlier Wolverine patterns paragraph and the rule at
line 1348 to reflect that distinction, or (B) align the example by
moving/renaming the example handler placement and path to the Application layer
and update all mentions (Wolverine patterns paragraph, Cross-module data pattern
example, and the rule at line 1348) to consistently say "Domain event handlers
live in the Application layer"; ensure all references to FormStepAddedHandler
and the consuming module ownership language match the chosen convention.
Duplicate comments:
In@docs/playbooks/patterns.md:
- Line 1350: Replace the nonstandard phrase "needs derived state computed" in
the sentence "If the consuming module needs derived state computed (e.g., "is
the form referenced at all?"), compute it from the local copy at query time — do
not try to sync aggregated state." with a grammatically correct construction
such as "needs derived state to be computed" or "needs derived state computed at
query time" so the sentence reads e.g. "If the consuming module needs derived
state to be computed (e.g., 'is the form referenced at all?'), compute it from
the local copy at query time — do not try to sync aggregated state." Ensure
quotes and punctuation remain intact.
Nitpick comments:
In@docs/playbooks/patterns.md:
- Line 1356: Change the fenced code block language from "powershell" to "bash"
for the block that contains the grep command (grep -rn
"SqlQueryRaw|ExecuteSqlRaw|FromSqlRaw|ExecuteSqlInterpolated" src/Modules/
--include="*.cs") so the syntax highlighting matches the bash/grep content and
aligns with other grep examples in the document.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro Plus **Run ID**: `54f1e1ae-1ded-49c8-ab92-eb8d97bcdc4a` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 1b0b878996f84e8ae4fd1e4081f7f76ae0478da8 and f4293f951321cc8cddcea12801960658e60ecba1. </details> <details> <summary>⛔ Files ignored due to path filters (9)</summary> * `docs/diagrams/container.svg` is excluded by `!**/*.svg` * `docs/diagrams/module-overview.svg` is excluded by `!**/*.svg` * `docs/diagrams/system-context.svg` is excluded by `!**/*.svg` * `docs/epics/E01-platform-foundation/diagrams/tenant-provisioning.svg` is excluded by `!**/*.svg` * `docs/epics/E02-identity-access/diagrams/auth-flow.svg` is excluded by `!**/*.svg` * `docs/epics/E03-data-modeling/diagrams/data-model.svg` is excluded by `!**/*.svg` * `docs/epics/E04-workflow-builder/diagrams/workflow-model.svg` is excluded by `!**/*.svg` * `docs/epics/E05-form-builder/diagrams/form-model.svg` is excluded by `!**/*.svg` * `docs/epics/E06-workflow-engine/diagrams/execution-flow.svg` is excluded by `!**/*.svg` </details> <details> <summary>📒 Files selected for processing (19)</summary> * `CLAUDE.md` * `docs/diagrams/container.excalidraw` * `docs/diagrams/generate-diagrams.mjs` * `docs/diagrams/module-overview.excalidraw` * `docs/diagrams/system-context.excalidraw` * `docs/epics/E01-platform-foundation/diagrams/tenant-provisioning.excalidraw` * `docs/epics/E02-identity-access/diagrams/auth-flow.excalidraw` * `docs/epics/E03-data-modeling/diagrams/data-model.excalidraw` * `docs/epics/E04-workflow-builder/diagrams/workflow-model.excalidraw` * `docs/epics/E05-form-builder/diagrams/form-model.excalidraw` * `docs/epics/E06-workflow-engine/diagrams/execution-flow.excalidraw` * `docs/playbooks/patterns.md` * `src/Axis.Api/Infrastructure/AddFormFieldRequestConverter.cs` * `src/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Handlers/WorkflowArchivedHandler.cs` * `src/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Handlers/WorkflowPublishedHandler.cs` * `src/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Handlers/WorkflowUnarchivedHandler.cs` * `src/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure/Handlers/WorkflowPublishedHandler.cs` * `tests/Api/Axis.Api.Tests/FormBuilder/FormEndpointTests.cs` * `tests/Api/Axis.Api.Tests/Helpers/ApiTestFixture.cs` </details> <details> <summary>✅ Files skipped from review due to trivial changes (6)</summary> * docs/epics/E03-data-modeling/diagrams/data-model.excalidraw * docs/diagrams/container.excalidraw * docs/diagrams/system-context.excalidraw * docs/epics/E06-workflow-engine/diagrams/execution-flow.excalidraw * docs/epics/E01-platform-foundation/diagrams/tenant-provisioning.excalidraw * docs/epics/E02-identity-access/diagrams/auth-flow.excalidraw </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (10)</summary> * src/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Handlers/WorkflowPublishedHandler.cs * docs/diagrams/module-overview.excalidraw * src/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Handlers/WorkflowUnarchivedHandler.cs * src/Modules/FormBuilder/Axis.FormBuilder.Infrastructure/Handlers/WorkflowArchivedHandler.cs * src/Modules/WorkflowEngine/Axis.WorkflowEngine.Infrastructure/Handlers/WorkflowPublishedHandler.cs * CLAUDE.md * src/Axis.Api/Infrastructure/AddFormFieldRequestConverter.cs * tests/Api/Axis.Api.Tests/FormBuilder/FormEndpointTests.cs * tests/Api/Axis.Api.Tests/Helpers/ApiTestFixture.cs * docs/diagrams/generate-diagrams.mjs </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Remove all var declarations in the integration test suite and replace with explicit types. Also removes the ! null-forgiving workaround introduced in the previous commit — ReadFromJsonAsync<JsonElement> can be assigned directly to JsonElement without the operator, consistent with AuthHelper.cs. Affected files: FormEndpointTests, WorkflowEndpointTests, ModelEndpointTests, RecordEndpointTests, RoleEndpointTests, UserEndpointTests, OrganizationEndpointTests. Also inlines viewerRole usage in OrganizationEndpointTests to avoid importing the Identity domain type into the test file. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the inlined .Id extraction (which avoided importing the domain type) with the proper explicit Role local variable and a using directive. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tives Documents the pitfall of inlining or restructuring code to avoid importing a type when replacing var with explicit types — which is a workaround, not a fix. Adds to Code hygiene checklist as item 2 with a concrete before/after example from OrganizationEndpointTests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CLAUDE.md: - Git rule: PR review fixes go to the existing PR branch, never a new branch - Violation sweep rule: clarify scope is the entire relevant layer/file type, not just the reported file patterns.md (Cross-module data): - Add rule: Wolverine event handlers must filter by OrganizationId in addition to the entity ID for correct tenant isolation patterns.md (Command idempotency): - Add Pattern 3: try-catch DbUpdateException for concurrent INSERT race under Wolverine at-least-once delivery (check-before-insert only handles sequential duplicates, not parallel handler invocations) patterns.md (Code hygiene checklist): - Add item 3: JsonElement from ReadFromJsonAsync needs no ! operator - Fix duplicate item numbering (two items were numbered 3) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Previous entries were too incident-specific (concrete class names, file paths, technology-specific examples in rule text). Rewritten to express the underlying principle so they apply to future analogous situations: - CLAUDE.md violation sweep: remove incident-level examples, keep principle - patterns.md OrganizationId rule: remove inline code from bullet, state intent - patterns.md DbUpdateException: remove enumerated conditions, keep the when - patterns.md ! operator: reframe as "verify before adding !" rather than JsonElement-specific gotcha Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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)
docs/playbooks/patterns.md (1)
1262-1262:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftResolve contradictory guidance on event handler layer placement.
The document contradicts itself on where cross-module event handlers belong:
- Line 1262: "Domain event handlers live in the Application layer of the consuming module; never in Domain or Infrastructure."
- Line 1316: Example places
FormStepAddedHandlerinFormBuilder.Infrastructure/Handlers/- Line 1348: "The consuming module (A) owns the handler and the local copy table — both in A's Infrastructure layer."
Based on learnings, cross-module data-synchronization handlers that update local denormalized tables touch Infrastructure concerns (DbContext, table access). If that's the intent, update Line 1262 to clarify: "Domain event handlers that implement business logic live in the Application layer. Handlers that synchronize cross-module denormalized data live in Infrastructure alongside the tables they update."
Alternatively, if all handlers belong in Application, move the example to
FormBuilder.Application/EventHandlers/and update Line 1348.Based on learnings: "Match existing local conventions: within a module or file, prefer consistency with patterns already present unless they directly conflict with CLAUDE.md or an official playbook."
Also applies to: 1316-1316, 1348-1348
🤖 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 `@docs/playbooks/patterns.md` at line 1262, Update the contradictory guidance by changing the sentence at Line 1262 to clarify the two cases: state that "Domain event handlers that implement business logic belong in the Application layer; event handlers whose primary responsibility is cross‑module data synchronization (e.g., updating local denormalized tables / DbContext) belong in the Infrastructure layer alongside the tables they update." Then make the example and its description consistent: either move FormStepAddedHandler into FormBuilder.Infrastructure/Handlers/ (and keep Line 1348's phrasing that the consuming module owns the handler and local copy table in Infrastructure) or, if you prefer the Application rule, move the example into FormBuilder.Application/EventHandlers/ and change Line 1348 to say the consuming module owns the handler and local copy table in Application; ensure references to FormStepAddedHandler, local copy table, and DbContext match the chosen placement.
♻️ Duplicate comments (1)
docs/playbooks/patterns.md (1)
1351-1351:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix nonstandard grammar in rule.
"needs derived state computed" is nonstandard. Change to either "needs derived state to be computed" or "needs to compute derived state."
📝 Suggested fix
-- If the consuming module needs derived state computed (e.g., "is the form referenced at all?"), compute it from the local copy at query time — do not try to sync aggregated state. +- If the consuming module needs derived state to be computed (e.g., "is the form referenced at all?"), compute it from the local copy at query time — do not try to sync aggregated state.🤖 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 `@docs/playbooks/patterns.md` at line 1351, The sentence uses nonstandard phrasing "needs derived state computed"; update it to a standard form such as "needs derived state to be computed" (or alternatively "needs to compute derived state") so the rule reads: "If the consuming module needs derived state to be computed (e.g., 'is the form referenced at all?'), compute it from the local copy at query time — do not try to sync aggregated state."
🧹 Nitpick comments (1)
docs/playbooks/patterns.md (1)
1389-1416: ⚡ Quick winConsider narrowing the exception handling in Pattern 3.
The catch block swallows all
DbUpdateExceptioninstances, but only unique constraint violations indicate a harmless duplicate-delivery race. Other database errors (foreign key violations, serialization failures, connection issues) would also be silently ignored.Consider either:
- Checking
ex.InnerExceptionfor the specific PostgreSQL unique constraint error code (23505)- At minimum, logging the exception before swallowing it to aid debugging
Current pattern is safe for correctness (transaction rolls back), but may hide real bugs.
♻️ Proposed enhancement
catch (DbUpdateException ex) { - // Concurrent duplicate event delivery — row already inserted by a parallel invocation. + // Concurrent duplicate event delivery — row already inserted by a parallel invocation. + // Log to distinguish from other DB errors. + logger.LogInformation(ex, "Duplicate event delivery for WorkflowId {WorkflowId}", `@event.WorkflowId`); }🤖 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 `@docs/playbooks/patterns.md` around lines 1389 - 1416, The catch-all DbUpdateException in Handle(WorkflowPublished, CancellationToken) hides non-unique-constraint DB errors; change the catch to inspect the exception and only swallow the duplicate-insert case (Postgres SQLSTATE 23505) while logging and re-throwing or failing for other errors. Specifically, catch DbUpdateException as ex around the await SaveChangesAsync(ct), examine ex.InnerException (or the provider-specific exception) for SQLSTATE "23505" (unique constraint), and treat that case as the benign duplicate-delivery; for all other exceptions, call process/diagnostic logger with the exception details and rethrow or propagate the error instead of silently swallowing it.
🤖 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 `@docs/playbooks/patterns.md`:
- Line 1262: Update the contradictory guidance by changing the sentence at Line
1262 to clarify the two cases: state that "Domain event handlers that implement
business logic belong in the Application layer; event handlers whose primary
responsibility is cross‑module data synchronization (e.g., updating local
denormalized tables / DbContext) belong in the Infrastructure layer alongside
the tables they update." Then make the example and its description consistent:
either move FormStepAddedHandler into FormBuilder.Infrastructure/Handlers/ (and
keep Line 1348's phrasing that the consuming module owns the handler and local
copy table in Infrastructure) or, if you prefer the Application rule, move the
example into FormBuilder.Application/EventHandlers/ and change Line 1348 to say
the consuming module owns the handler and local copy table in Application;
ensure references to FormStepAddedHandler, local copy table, and DbContext match
the chosen placement.
---
Duplicate comments:
In `@docs/playbooks/patterns.md`:
- Line 1351: The sentence uses nonstandard phrasing "needs derived state
computed"; update it to a standard form such as "needs derived state to be
computed" (or alternatively "needs to compute derived state") so the rule reads:
"If the consuming module needs derived state to be computed (e.g., 'is the form
referenced at all?'), compute it from the local copy at query time — do not try
to sync aggregated state."
---
Nitpick comments:
In `@docs/playbooks/patterns.md`:
- Around line 1389-1416: The catch-all DbUpdateException in
Handle(WorkflowPublished, CancellationToken) hides non-unique-constraint DB
errors; change the catch to inspect the exception and only swallow the
duplicate-insert case (Postgres SQLSTATE 23505) while logging and re-throwing or
failing for other errors. Specifically, catch DbUpdateException as ex around the
await SaveChangesAsync(ct), examine ex.InnerException (or the provider-specific
exception) for SQLSTATE "23505" (unique constraint), and treat that case as the
benign duplicate-delivery; for all other exceptions, call process/diagnostic
logger with the exception details and rethrow or propagate the error instead of
silently swallowing it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 406e2711-9c2d-4f56-ae22-1c2dc7d74ad9
📒 Files selected for processing (9)
CLAUDE.mddocs/playbooks/patterns.mdtests/Api/Axis.Api.Tests/DataModeling/ModelEndpointTests.cstests/Api/Axis.Api.Tests/DataModeling/RecordEndpointTests.cstests/Api/Axis.Api.Tests/FormBuilder/FormEndpointTests.cstests/Api/Axis.Api.Tests/Identity/OrganizationEndpointTests.cstests/Api/Axis.Api.Tests/Identity/RoleEndpointTests.cstests/Api/Axis.Api.Tests/Identity/UserEndpointTests.cstests/Api/Axis.Api.Tests/WorkflowBuilder/WorkflowEndpointTests.cs
✅ Files skipped from review due to trivial changes (2)
- tests/Api/Axis.Api.Tests/DataModeling/RecordEndpointTests.cs
- tests/Api/Axis.Api.Tests/DataModeling/ModelEndpointTests.cs
Implement all 24 findings from the doc quality audit: - CLAUDE.md: fix OwnsMany ParentId placeholder (was wrong, could cause EF shadow column bug); remove incident-specific parenthetical from Agent Integrity Rules; simplify no-inline-type rule; rewrite rate limiting rule as principle (remove hardcoded paths); OpenAPI rule now references the status code mapping table instead of listing codes inline - patterns.md: fix stale wireframe path (pre-restructure docs/wireframes/E0N path → correct docs/epics/*/wireframes path); remove rescue-branch procedure (already in CLAUDE.md); replace DataModel/FieldDefinition with generic MyAggregate/ItemDto in JSONB example; remove "removed from codebase" retraction from cross-module anti-pattern; rework NuGet rules to remove version-specific coupling examples; replace Role/Axis.Identity.Domain in Code hygiene §2 with generic types; broaden null-forgiving ! example in §3; add preamble to OwnsMany example; remove specific Docker endpoint from hardcoding rule; add generalization label to FormBuilder cross-module example; add preamble note to Pattern 3 DbUpdateException; replace Role/Name=null! with generic in EF Core aggregate mapping; remove specific Wolverine API names from forward-looking durable-outbox note; fix numbered duplicate items in hygiene checklist - testing.md: pre-commit gate references CLAUDE.md Gate 1 as canonical; ApiTestFixture section rewritten with generic types (no specific DB names/connection strings); Identity exception explained as public-schema principle not OpenIddict implementation detail - PROGRESS.md: stripped to layer-status summaries only (no class names, connection strings, or implementation detail that will drift with every PR) - wireframes.md: screen inventory table now explicitly cross-references generate-template.mjs as authoritative source to prevent the two from drifting silently Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Gate 2 — two new rows: - "Moved or renamed a wireframe/diagram/source folder" → search all docs/ for stale paths and update cross-references including code examples in patterns.md (closes the recurring drift that caused the stale wireframe path in Finding 14) - "Completed a full layer for a module" expanded: PROGRESS.md must be one short paragraph per module, layer-status only — no class names, endpoint lists, or per-PR implementation detail (closes the format drift found in Finding 18) Gate 3 — question 7: - "Did I embed a library version, endpoint URL, or production class name in rule text?" If yes, move to a labelled example block with a generalization note. Rule text states the principle; specifics belong only in illustrative examples. (closes Findings 8, 21) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Root cause: Gate 2 was a silent mental checklist — agents could skip it without leaving any trace. Rules that require only mental work can be skipped invisibly; rules that require written output cannot. Two changes: 1. P0: add "Never commit without a completed, written Gate 2 walk-through — a mental check is not a walk-through". Makes the absence of the output a P0 violation, enforceable by any agent reading CLAUDE.md without needing hooks or reminders. 2. Gate 2 instruction: replace "go through and answer explicitly" with a required written format (Gate 2: row → triggered/not triggered). An agent that skips this now violates an explicit output requirement, not just a process suggestion. This closes the multi-agent maintenance gap: Stop hooks only apply to individual sessions; written-output rules apply to every agent that reads CLAUDE.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 (2)
docs/playbooks/patterns.md (2)
1563-1563:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix section numbering: should be "### 6." not "### 4."
There are already 5 numbered sections above this one (1. No inline fully-qualified type names, 2. No restructuring to avoid a
usingdirective, 3. Verify!is actually needed, 4. No scaffold placeholder files, 5. User input flowing into external identifiers), so "No direct commits tomain" should be section 6.📝 Proposed fix
-### 4. No direct commits to `main` +### 6. No direct commits to `main`🤖 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 `@docs/playbooks/patterns.md` at line 1563, Update the section heading for "No direct commits to `main`" from "### 4." to "### 6." so the numbering follows the five preceding sections; locate the heading text "No direct commits to `main`" in docs/playbooks/patterns.md and change its Markdown level-three numeric prefix accordingly.
1264-1264:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve layer placement inconsistency for cross-module sync handlers.
The document contradicts itself on where cross-module synchronization event handlers should be placed:
- Line 1264 (Wolverine patterns): "Domain event handlers live in the Application layer of the consuming module; never in Domain or Infrastructure."
- Line 1318 (Cross-module data pattern example):
FormBuilder.Infrastructure/Handlers/FormStepAddedHandler.cs- Line 1350 (Cross-module data pattern rule): "The consuming module (A) owns the handler and the local copy table — both in A's Infrastructure layer."
This is the same inconsistency flagged in a past review comment (marked as "Addressed" but still present in the current code). The document needs to pick a single convention:
Option A: State that data-synchronization handlers (cross-module sync, local read-model updates) belong in Infrastructure, while business-logic domain event handlers belong in Application. Update line 1264 to reflect this distinction.
Option B: Move all handlers to Application layer and update lines 1318 and 1350 accordingly.
Which approach aligns with the project's architecture?
Also applies to: 1318-1318, 1350-1350
🤖 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 `@docs/playbooks/patterns.md` at line 1264, The document contradicts itself about handler placement; adopt Option A: clarify that domain (business-logic) event handlers live in the Application layer while data-synchronization/local read-model update handlers belong in Infrastructure. Update the sentence at line 1264 to state this distinction explicitly, and adjust the examples and rules referencing FormBuilder.Infrastructure/Handlers/FormStepAddedHandler.cs and the rule at line 1350 so they consistently describe cross-module sync handlers as Infrastructure-owned (including the local copy table) while reserving Application for pure domain logic handlers.
♻️ Duplicate comments (1)
docs/playbooks/patterns.md (1)
1353-1353:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix grammar: "needs derived state computed" → "needs derived state to be computed".
The phrase is grammatically awkward. Based on the static analysis hint, change "needs derived state computed" to "needs derived state to be computed".
📝 Proposed fix
- - If the consuming module needs derived state computed (e.g., "is the form referenced at all?"), compute it from the local copy at query time — do not try to sync aggregated state. + - If the consuming module needs derived state to be computed (e.g., "is the form referenced at all?"), compute it from the local copy at query time — do not try to sync aggregated state.🤖 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 `@docs/playbooks/patterns.md` at line 1353, Update the sentence that currently reads "If the consuming module needs derived state computed (e.g., "is the form referenced at all?"), compute it from the local copy at query time — do not try to sync aggregated state." by changing "needs derived state computed" to "needs derived state to be computed" so the full sentence becomes: "If the consuming module needs derived state to be computed (e.g., "is the form referenced at all?"), compute it from the local copy at query time — do not try to sync aggregated state." Ensure you only change that phrase and preserve the example and dash punctuation.
🤖 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 `@docs/playbooks/patterns.md`:
- Line 1563: Update the section heading for "No direct commits to `main`" from
"### 4." to "### 6." so the numbering follows the five preceding sections;
locate the heading text "No direct commits to `main`" in
docs/playbooks/patterns.md and change its Markdown level-three numeric prefix
accordingly.
- Line 1264: The document contradicts itself about handler placement; adopt
Option A: clarify that domain (business-logic) event handlers live in the
Application layer while data-synchronization/local read-model update handlers
belong in Infrastructure. Update the sentence at line 1264 to state this
distinction explicitly, and adjust the examples and rules referencing
FormBuilder.Infrastructure/Handlers/FormStepAddedHandler.cs and the rule at line
1350 so they consistently describe cross-module sync handlers as
Infrastructure-owned (including the local copy table) while reserving
Application for pure domain logic handlers.
---
Duplicate comments:
In `@docs/playbooks/patterns.md`:
- Line 1353: Update the sentence that currently reads "If the consuming module
needs derived state computed (e.g., "is the form referenced at all?"), compute
it from the local copy at query time — do not try to sync aggregated state." by
changing "needs derived state computed" to "needs derived state to be computed"
so the full sentence becomes: "If the consuming module needs derived state to be
computed (e.g., "is the form referenced at all?"), compute it from the local
copy at query time — do not try to sync aggregated state." Ensure you only
change that phrase and preserve the example and dash punctuation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 16939da4-9fce-4c5d-9b2a-a7640ad560ed
📒 Files selected for processing (5)
CLAUDE.mddocs/PROGRESS.mddocs/playbooks/patterns.mddocs/playbooks/testing.mddocs/playbooks/wireframes.md
✅ Files skipped from review due to trivial changes (3)
- docs/playbooks/wireframes.md
- docs/PROGRESS.md
- CLAUDE.md
Extends the same "written output, not mental check" principle from Gate 2 to the remaining three gaps: Gate 3 — add required written output format (yes/no per question, action stated for each yes). "Answer explicitly" was still mental; the output block is not. Add Gate 3 to P0 alongside Gate 2 — same rule, same enforcement level. Code quality checks — replace the three text-only bullets (no TODO, no placeholder, no commented code) with a single grep command that must return empty output. Command output is observable; a mental "I checked" is not. Step 4 (AC mapping) — "map every AC before writing any code" now explicitly says "write this mapping out — do not keep it mental." The written map is the artifact that proves requirements were understood before coding began. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Gate 1 (build + tests must be green) was already described as a non-negotiable hard failure in the Non-Negotiable Gates section but was absent from the P0 quick reference list — while Gates 2 and 3 were just added there. All three gates are symmetric hard stops; P0 now reflects that consistently. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
3 tasks
This was referenced May 20, 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.
Summary
roughness:1, Virgil font), orthogonal arrow routing viaroutedArrow(); fix layout spacing in top-level diagrams (container, module-overview, system-context)docs/wireframes/E0X-*/intodocs/epics/E0X-*/wireframes/— each epic is now self-contained (features + diagrams + wireframes); update all 28 feature file links, generator scripts, and docsWhat changed
Cross-module violations (
src/)Diagrams (
docs/diagrams/)docs/diagrams/generate-diagrams.mjsroutedArrow()helper for orthogonal multi-segment arrows (no diagonal lines)Wireframe restructure (
docs/wireframes/,docs/epics/)git mv docs/wireframes/E0X-name/ → docs/epics/E0X-name/wireframes/(50 files, history preserved)../../../wireframes/E0X-name/screen→../wireframes/screengenerate-screens.mjswrite helper auto-routes epic paths to new locationgenerate-wireframes.ps1now scans bothdocs/wireframes/anddocs/epics/*/wireframes/app-shellmoved todocs/wireframes/root (it's a layout reference, not a screen);_shared/removedTest plan
node docs/diagrams/generate-diagrams.mjsruns without errors.\docs\scripts\generate-diagrams.ps1produces all 9 SVGsnode docs/wireframes/generate-screens.mjswrites files todocs/epics/*/wireframes/.\docs\scripts\generate-wireframes.ps1picks up all epic wireframes../wireframes/paths resolve correctly)Pending (next session)
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores
Tests