Result-based error handling for Catalog/Auth; remove Tags from frontend - #69
Closed
evertonschuster wants to merge 9 commits into
Closed
Result-based error handling for Catalog/Auth; remove Tags from frontend#69evertonschuster wants to merge 9 commits into
evertonschuster wants to merge 9 commits into
Conversation
Remove the standalone useCategories hook and inline its logic into useCategoriesListPage. Add create/update/delete handlers and list state (useAsync, mutate, captureGeneration) directly in the page hook. Introduce CategoriesEditorContext in the types file and update CategoryEditorDialog to consume the new context type. Also adjust onRetry to call execute(). This refactor removes indirection, consolidates list + editor mutations, and preserves existing behavior.
docs/adr/ (31 backend ADRs) was removed without updating any of the 100+ references to it across AGENTS.md, skills, and docs, which broke the governance check that verifies every docs/adr/NNNN reference resolves. Restoring the files keeps the documented rationale behind existing backend rules intact. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Business/presentation code no longer throws for expected outcomes or relies on try/catch as its primary error-handling mechanism (ADR 014, ADR 015): domain entities (Category, Tag, Session, User, Tenant), infrastructure mappers, useAsync, and every Catalog/Auth hook branch on Result directly instead. Converting mapOidcUserToSession/mapCategoryDtoToDomain surfaced a real gap along the way - a malformed cached session could throw uncaught and get silently misread as "not logged in" - now closed. Adds a global error-capture net (ErrorReporter port + React 19's onCaughtError/onUncaughtError + window unhandledrejection/error listeners) for whatever still throws unexpectedly, ready to wire to a real observability backend later. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Removes the entire Tags vertical from apps/admin-frontend (domain, application, infrastructure, presentation, MSW handlers, E2E specs, nav entry, route, and catalog facade wiring) while intentionally retaining the backend Tag domain entity and /api/v1/tags endpoints, including Service's many-to-many relationship to Tag - a project-owner decision documented in docs/adr/016-remove-tags-frontend.md. Updates STATUS.md, DOMAIN.md, API.md, apps/admin-frontend/AGENTS.md, .agent.md, and the canonical agenza-frontend-feature skill (synced to .claude/skills/ and .agents/skills/) so Categories replaces Tags as the reference CRUD implementation throughout, and so the docs are explicit that the backend still returns tags/tagIds on ServiceDto even though the frontend has no Tag type to represent it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 244 files, which is 144 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (244)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Aug 2, 2026
Closed
Owner
Author
|
Fechando em favor da divisão em PRs menores para revisão com CodeRabbit:
Todas as 4 verificadas com build/lint/format/test reais antes do push. #71 empilhada sobre #70... na verdade #71 é independente (base main); #72 empilhada sobre #71; #73 empilhada sobre #72. |
Owner
Author
evertonschuster
added a commit
that referenced
this pull request
Aug 2, 2026
Admin.SharedKernel.AspNetCore's ResultExtensions.ToActionResult now wraps every successful Result<TValue> response in an ApiResponse<T> envelope (data/success/timestamp/traceId/correlationId) - this PR's own change. scripts/smoke_oidc_contract.py wasn't updated to match, so its tenant- provisioning assertion read the old flat shape and failed even though the endpoint worked correctly (201, real tenantId, just nested under "data" now). The three assertions checking failure responses (401/403 "code" field) are unaffected - those go through ToProblemResult, which was not wrapped and never changed shape. This bug predates this PR split - it already failed identically on the original, unsplit PR (#69) before any of this work was divided up. Verified against a real locally-running Aspire stack: both `npm run generate:api-types:check` and `python scripts/smoke_oidc_contract.py` pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
evertonschuster
added a commit
that referenced
this pull request
Aug 2, 2026
…ponse/CORS infra (#70) * Remove local git hooks, add GetCategoryById endpoint, backend API response/CORS infra Repo tooling: removes .husky/pre-commit and apps/admin-frontend/.lintstagedrc.json in favor of explicit quality-gate commands (docs/adr/0031); updates the trunk-based-workflow and branch-agnostic-precommit ADRs accordingly. Backend: adds GetCategoryById query/handler/endpoint for Categories; introduces ApiResponse/ApiProblemDetailsFactory and CORS/OpenAPI documentation setup extensions shared across identity-service and services-service; updates architecture_guard.py's database-boundary check to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Regenerate services-api.d.ts to match the GetCategoryById endpoint This PR added GET /api/v1/categories/{id} to the backend but didn't update the frontend's generated OpenAPI types, so CI's api-contract-check correctly failed - it regenerates services-api.d.ts from the live backend and diffs against the checked-in version. Regenerated against this PR's own backend (npm run generate:api-types), verified clean with npm run generate:api-types:check, and re-ran build/lint/format to confirm nothing downstream broke. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix smoke_oidc_contract.py for the new ApiResponse<T> success envelope Admin.SharedKernel.AspNetCore's ResultExtensions.ToActionResult now wraps every successful Result<TValue> response in an ApiResponse<T> envelope (data/success/timestamp/traceId/correlationId) - this PR's own change. scripts/smoke_oidc_contract.py wasn't updated to match, so its tenant- provisioning assertion read the old flat shape and failed even though the endpoint worked correctly (201, real tenantId, just nested under "data" now). The three assertions checking failure responses (401/403 "code" field) are unaffected - those go through ToProblemResult, which was not wrapped and never changed shape. This bug predates this PR split - it already failed identically on the original, unsplit PR (#69) before any of this work was divided up. Verified against a real locally-running Aspire stack: both `npm run generate:api-types:check` and `python scripts/smoke_oidc_contract.py` pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
evertonschuster
added a commit
that referenced
this pull request
Aug 2, 2026
…infra (#71) Split out of #69 (part 2 of N — see that PR for the full picture). - features/auth/: Session/User/Tenant.create() and every AuthRepository method (initiateLogin/handleCallback/logout) return Result<T, E> instead of throwing; OidcAuthRepository's own try/catch around oidc-client-ts stays as a contained infrastructure-adapter boundary. See docs/adr/015-auth-result-errors.md (landing in a later PR in this stack, since it documents Catalog too). - shared/: adds Result.ts (Result/success/failure/flatMapResult/ combineResults), ErrorReporter port + ConsoleErrorReporter adapter, and rewrites useAsync.ts to take () => Promise<Result<T, E>> instead of a throwing () => Promise<T>. - test/fixtures/: adds authEntityFixtures.ts (shadow Tenant/User/Session fixtures that unwrap the Result so call sites read like before) and unwrapResult.ts. Temporary compatibility shims (flagged for removal in the next PR in this stack, which migrates Catalog to Result end-to-end and removes Tags): useCategories.ts/useServices.ts/useTags.ts wrap their existing list-fetch call in success()/failure() instead of letting it reject, and ~20 old Categories/Services/Tags test files that constructed Tenant/User directly now import the shadow fixtures instead — neither changes any other behavior, they only satisfy useAsync's new contract so this PR builds and tests green on its own without pulling in the unrelated Catalog reorg. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Merged
5 tasks
evertonschuster
added a commit
that referenced
this pull request
Aug 2, 2026
…ervices; physical reorg (#75) * Migrate Catalog (Categories/Services/Tags) to Result errors; remove Services vertical; physical reorg Split out of #69 (part 3 of 4 — see that PR for the full picture). Recreated from origin/main after #70 and #71 merged, since this repo's convention (and the split-large-coderabbit-pr skill) is a sequential series, not stacking on an unmerged branch — CodeRabbit also doesn't review PRs whose base isn't the default branch, so stacking silently skipped review for this and the next PR in the series. Same content as the original #72, just re-based; no functional change. This PR is larger than the <100-file target used for the other PRs in this series, deliberately — see "Why this couldn't be split further" below. - Categories, Services, and Tags all move to the Result-based error convention docs/adr/014 establishes: domain entities' create() methods, mappers, and API repositories return Result<T, AppError> instead of throwing; useAsync.ts (already Result-based, landed in #71) is the one hook every feature's data layer builds on now. - app/composition/container.ts's CatalogFacade drops the use-case-class indirection (ListCategories/CreateTag/etc. as separate classes) for direct repository delegation (`{ execute: repo.method }`) - there's no orchestration between the facade and the repository, so the extra class per operation wasn't earning its keep. The 24 now-orphaned use-case-class files (application/use-cases/{categories,services,tags}/) are deleted. - Services' frontend implementation (ServicesPage, ServiceForm, six ServicesPage.*.test.tsx files, all its components/hooks/models) is fully removed, reverting `/services` to a placeholder page (app/pages/ServicesPage/ServicesPage.tsx) - this vertical is going back to `stub` status, see docs/STATUS.md. - Categories moves to a routed create/edit dialog (features/catalog/presentation/categories/pages/CategoriesListPage/, .../CategoryEditorDialog/) per docs/adr/012, replacing the old flat CategoriesPage.tsx/useCategories.ts/CategoryEditorDialog.tsx shape. - Tags gets the equivalent internal move (hooks/useTagEditor.ts, pages/TagEditorDialog.tsx) and its own Result migration (Tag.ts/tagMapper.ts/ApiTagRepository.ts) - Tags itself is not being removed here, just migrated; its removal is a later PR in this stack (docs/adr/016). - shared/: AuthenticatedHttpClient's get/post/put/delete now return Result<T, AppError> instead of throwing; DeleteConfirmationDialog takes entityName/entityType instead of a raw title/description pair; useCreateInline is removed (no longer used once Services - its only consumer - is gone). - Also removes .husky/pre-commit and fixes architecture_guard.py's precommit check accordingly (already merged independently via #70; included here too since this branch's own ancestry needed it before #70 existed). ## Why this couldn't be split further I initially tried a narrower "Category-only foundation" PR (~99 files) deferring Services/Tags. That failed a real build: app/composition/ container.ts wires TagRepository with its *new* method signature directly (tagRepository.listAll(options) instead of the old (tenantContext, options) two-arg form) - not just a return-type change useAsync-style, but the interface itself. Making that build without also migrating TagRepository/ApiTagRepository/tagMapper/Tag.ts for real isn't a smaller wrapper shim - it's the same size of work as just finishing the migration, since there's no reduced version of an interface signature. Categories, Services, and Tags share container.ts's catalog wiring, router.tsx, and AuthenticatedHttpClient tightly enough that they're one atomic, verified-buildable unit at this layer - mirroring why Auth couldn't be split from Catalog either, just one layer down. ## Test plan - [x] `npm install` + `npm run build --workspace=apps/admin-frontend` — green - [x] `npm run lint --workspace=apps/admin-frontend` — clean, 0 warnings - [x] `npm run format:check --workspace=apps/admin-frontend` — clean - [x] `npm run test --workspace=apps/admin-frontend` — 368/368 passing - [x] `scripts/sync_agent_skills.py --check`, `scripts/check_agent_governance.py`, `scripts/architecture_guard.py` — all pass Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix categories-mobile.spec.ts's mock for the GET-by-id endpoint useCategoryEditor fetches its own category via GET /api/v1/categories/{id} (docs/adr/013), but this spec's route mock matched any /api/v1/categories* path and always returned the full list array regardless of whether the request was for the collection or a single id - so the by-id fetch received an array instead of a CategoryDto, and the edit dialog's Nome field never populated. Mock now inspects the last path segment and returns the matching single category (404 if not found) for a by-id GET, the full list otherwise. Verified against the real Playwright suite (production build + preview, matching CI): all 10 e2e specs pass, including this one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
6 tasks
evertonschuster
added a commit
that referenced
this pull request
Aug 2, 2026
Split out of #69 (final part of this series — see that PR for the full picture). Recreated from origin/main after #70/#71/#75 merged, since this repo's convention (and the split-large-coderabbit-pr skill) is a sequential series, not stacking on an unmerged branch. Same content as the original #73, just re-based; no functional change. Removes the entire Tags vertical from apps/admin-frontend (domain, application, infrastructure, presentation, MSW handlers, E2E specs, nav entry, route, and catalog facade wiring) while intentionally retaining the backend Tag domain entity and /api/v1/tags endpoints, including Service's many-to-many relationship to Tag - a project-owner decision, see docs/adr/016-remove-tags-frontend.md. Categories replaces Tags as the reference CRUD implementation throughout the docs and the agenza-frontend-feature skill. ## Test plan - [x] `npm install` + `npm run build --workspace=apps/admin-frontend` — green - [x] `npm run lint --workspace=apps/admin-frontend` — clean, 0 warnings - [x] `npm run format:check --workspace=apps/admin-frontend` — clean - [x] `npm run test --workspace=apps/admin-frontend` — 305/305 passing - [x] `npx playwright test` (full e2e suite, production build + preview) — 8/8 passing - [x] `scripts/sync_agent_skills.py --check`, `scripts/check_agent_governance.py`, `scripts/architecture_guard.py` — all pass Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
4 tasks
evertonschuster
added a commit
that referenced
this pull request
Aug 2, 2026
…#77) These 3 files (agent-skills/agenza-frontend-feature/SKILL.md and its two synced copies under .claude/skills/ and .agents/skills/) live at the repo root, outside apps/admin-frontend/ — every diff I computed while splitting #69 into #70/#71/#75/#76 was scoped to apps/admin-frontend (and backend/ for #70), so these files' accumulated updates from this session (Catalog Result migration, Auth Result migration, and finally the Tags-removal doc pass replacing TagsPage/TagForm with Categories as the reference implementation) never made it into any of the split PRs, even though the actual code changes they describe are all correctly merged. Content taken directly from the original branch's final commit (4911abb), already reviewed and governance-checked at the time. Verified again here against the current merged main: sync_agent_skills.py --check, check_agent_governance.py, and architecture_guard.py all pass, and the file paths the skill references (CategoriesListPage.tsx, CategoryForm.tsx, categoryMapper.ts, AdminLayout.tsx) all exist in the current tree. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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
features/catalog/(Categories/Tags) andfeatures/auth/to athrow-free,
Result<T, E>-based error-handling convention end to end(domain entities, mappers, repositories, hooks, use cases), plus a
global error-capture net (
onCaughtError/onUncaughtError,unhandledrejection/errorlisteners) wired through a pluggableErrorReporterport — seedocs/adr/014anddocs/adr/015.GET /api/v1/categories/{id}and switcheduseCategoryEditortofetch its own category directly instead of sharing state through
useOutletContext— seedocs/adr/013.infrastructure, presentation, MSW handlers, E2E specs, nav entry, route)
while intentionally keeping the backend
Tagdomain entity and/api/v1/tagsendpoints untouched, includingService's many-to-manyrelationship to
Tag— a project-owner decision, seedocs/adr/016-remove-tags-frontend.md. Categories replaces Tags as thereference CRUD implementation throughout the docs and the
agenza-frontend-featureskill.Test plan
dotnet build backend/AdminBackend.slnx/dotnet test backend/AdminBackend.slnx— greennpm run format:check/lint/build/test:coverage(apps/admin-frontend) — green, 305/305 tests, coverage above the 85/85/80/80 gatescripts/sync_agent_skills.py --check,scripts/check_agent_governance.py,scripts/architecture_guard.py— all pass🤖 Generated with Claude Code