Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds extensive planning, research, and architecture documentation, introduces README-driven type-safety tests, removes a redundant breadcrumb type-safety test, and changes the Try.breadcrumbs API and related PositionalBreadcrumbs type to support stricter key validation and new configuration overloads. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5eec5ad02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export type PositionalBreadcrumbs<TArgs extends readonly unknown[]> = readonly ( | ||
| | string | ||
| | readonly string[] | ||
| | BreadcrumbExtractor<TArgs> |
There was a problem hiding this comment.
Allowing readonly string[] inside PositionalBreadcrumbs means callers can now pass unvalidated keys for object parameters, e.g. breadcrumbs([['id','nam']]) compiles even when the argument type only has id/name. This regresses the type-safety guarantee for breadcrumb keys and can silently drop invalid keys at runtime. If positional string[] is needed, it should still be validated per-parameter (e.g., via a ValidateKeys-style mapping or requiring the { param, keys } extractor form).
Useful? React with 👍 / 👎.
Signed-off-by: w01fgang <sumin@unix-center.ru>
Applied consistent Prettier formatting across the entire codebase following the dependency upgrade to Vitest v4. Fixed a test expectation in flexible-breadcrumbs.test.ts where the transform function was updated but the assertion wasn't updated to match. Changes: - Applied Prettier formatting to all source files - Fixed test assertion to match transform output (priority vs param2_value) - All tests pass (71 tests across 2 test suites) - No security vulnerabilities detected 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- STACK.md - Technologies and dependencies - ARCHITECTURE.md - System design and patterns - STRUCTURE.md - Directory layout - CONVENTIONS.md - Code style and patterns - TESTING.md - Test structure - INTEGRATIONS.md - External services - CONCERNS.md - Technical debt and issues
TypeScript Try utility with Sentry reporting across environments
Mode: yolo Depth: quick Parallelization: enabled Workflow agents: research=on, plan_check=on, verifier=on
Files: - STACK.md - FEATURES.md - ARCHITECTURE.md - PITFALLS.md - SUMMARY.md Key findings: - Stack: TS 5.9 + Sentry 10.38 + Rollup 4. - Architecture: core Try/Result + reporter interface + env adapters. - Critical pitfall: silent error swallowing without explicit report.
13 requirements across 5 categories 2 requirements deferred to v2
Report only on explicit report(); always record breadcrumbs
Phases: 1. Core Try Semantics: TRY-01..04 2. Reporting + Runtime Entry Points: SENT-01..04, ENTRY-01..03, DIAG-01..02 3. Documentation & Examples: DX-01 All v1 requirements mapped to phases.
- add expectTypeOf coverage for value/default/error/unwrap - add ts-expect-error cases for invalid args and breadcrumbs
Tasks completed: 1/1 - Add README-driven type-safety tests SUMMARY: .planning/quick/001-write-tests-that-verify-type-safety-acco/001-SUMMARY.md
…e use cases covered in Readme.md Quick task completed. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/types.ts (1)
151-157:⚠️ Potential issue | 🟡 MinorClarify comment regarding string[] inclusion in BreadcrumbOptions.
The comment at lines 152-153 states that
string[]arrays are "intentionally excluded to force validation through ValidateKeys." However,PositionalBreadcrumbs<TArgs>(defined at line 136-139 and included at line 157) explicitly containsreadonly string[]as one of its union members.The distinction appears to be semantic:
ValidateKeysvalidates string[] keys passed to thebreadcrumbs()method against the object structure, whereas string[] withinPositionalBreadcrumbsentries serve as position-specific key extractors. Clarify the comment to explain this difference or update it to accurately reflect that positional breadcrumb syntax does include string[] with different validation semantics.
🤖 Fix all issues with AI agents
In @.planning/codebase/INTEGRATIONS.md:
- Around line 43-44: Update the "**CI Pipeline:**" section in
.planning/codebase/INTEGRATIONS.md to reflect the actual GitHub Actions CI by
replacing "None detected" with a short description that tests run in
.github/workflows/ci.yml (e.g., "GitHub Actions: .github/workflows/ci.yml — runs
unit/integration tests"), and optionally add a cross-reference to CONCERNS.md
where .github/workflows/ci.yml is mentioned to keep the docs consistent.
In @.planning/codebase/TESTING.md:
- Around line 26-27: Replace the mistaken glob pattern " * .test.ts" with the
correct "*.test.ts" in the TESTING.md naming convention section; search for any
other occurrences of " * .test.ts" in the file and update them to "*.test.ts" so
the intended test file naming pattern is accurate.
In @.planning/ROADMAP.md:
- Around line 62-71: Update the "Execution Order:" line to match the actual
defined phases instead of the leftover template sequence; replace the current
sequence "2 → 2.1 → 2.2 → 3 → 3.1 → 4" with the real phase order (e.g., "1 → 2 →
3") or otherwise reflect the phases shown in the table, and remove any template
placeholders; locate and edit the "Execution Order:" heading and the sequence
string in the ROADMAP.md content to keep the doc consistent.
In @.planning/STATE.md:
- Line 63: Fix the typo "accoriding" to "according" in the quick tasks table
entry string "write tests that verify type safety accoriding to the use cases
covered in Readme.md" (the table row shown in STATE.md); update that exact cell
text so it reads "write tests that verify type safety according to the use cases
covered in Readme.md" and ensure the same correction is applied to any identical
occurrences in the quick tasks table.
- Line 15: Fix the typo in the last activity entry by replacing "accoriding"
with "according" in the text "write tests that verify type safety accoriding to
the use cases covered in Readme.md" so the sentence reads "write tests that
verify type safety according to the use cases covered in Readme.md".
In `@src/__tests__/type-safety.test.ts`:
- Around line 69-88: The test's type assertion mismatches the declared type of
the headers parameter in processRequest: update the function signature of
processRequest (the function used when constructing new Try in the test) so its
headers parameter is typed as Record<'Content-Type', string> to match the
breadcrumbs callback, or alternatively change the test assertion inside the
breadcrumbs callback to assert the actual declared type (e.g., any or
Record<string,string>) instead of Record<'Content-Type', string>; locate
processRequest and the breadcrumb callback in the test to implement the chosen
fix.
In `@src/core/Try.ts`:
- Around line 227-229: The overload for breadcrumbs shadows the class generic T
with a local generic T causing the return type to reference the wrong generic;
rename the local generic (e.g., use U or Transformers) in the breadcrumbs
signature so it reads breadcrumbs<U extends
VariadicBreadcrumbTransformers<TArgs>>(...transformers: U): Try<T, TArgs> and
update any internal references to that local generic accordingly so the return
type uses the class-level T (the function return type) rather than the local
tuple type; ensure the symbols referenced are breadcrumbs, Try,
VariadicBreadcrumbTransformers, and TArgs.
🧹 Nitpick comments (6)
.planning/research/FEATURES.md (1)
51-66: Consider adding a language identifier to the code fence.The dependency diagram code block would benefit from a language identifier (e.g.,
textormermaidif converted to a diagram format) for better rendering and linting compliance.Suggested fix
-``` +```text [Sync Try wrapper] └──requires──> [Typed error channel].planning/codebase/CONCERNS.md (1)
23-23: Minor: Verify the coverage provider package version syntax.The suggested fix approach references
@vitest/coverage-v8@v4. The@v4tag may not be the correct way to specify the version—typically it would be@^4.0.0or similar semver range rather than@v4.Proposed fix
- Fix approach: `npm i -D `@vitest/coverage-v8`@v4`, update `vite.config.ts` with `coverage.provider: 'v8'`, thresholds: { statements: 90, etc. }, add to CI. + Fix approach: `npm i -D `@vitest/coverage-v8`@^4.0.0`, update `vite.config.ts` with `coverage.provider: 'v8'`, thresholds: { statements: 90, etc. }, add to CI..planning/codebase/INTEGRATIONS.md (1)
25-28: Formatting: Orphaned line under Auth Provider section.Line 28 appears to be an orphaned implementation detail that should either be integrated into the Auth Provider section or removed, as it's inconsistently indented.
Proposed fix
**Auth Provider:** - None - Library is integration-agnostic - - - Implementation: Optional SentryReporter adapters +- Implementation: Optional SentryReporter adapters for error reporting.planning/research/ARCHITECTURE.md (1)
48-69: Optional: Add language identifier to fenced code block.The code block at line 51 lacks a language identifier, which affects syntax highlighting.
Proposed fix
## Recommended Project Structure -``` +```text src/ ├── core/ # Result/Try primitives, combinators.planning/codebase/TESTING.md (1)
29-35: Consider updating the test file structure.The AI summary indicates that
src/__tests__/type-safety.test.tswas added in this PR, but it's not listed in the structure. Consider updating to reflect the complete test suite.Proposed fix
src/ └── __tests__/ ├── Try.test.ts - └── flexible-breadcrumbs.test.ts + ├── flexible-breadcrumbs.test.ts + └── type-safety.test.tssrc/__tests__/type-safety.test.ts (1)
110-114: Misleading comment:nameis valid,namis not.The comment states "name is not a valid key" but the actual invalid key being tested is
'nam'(a typo). The_customerobject has{ id, name }, sonamewould be valid. Consider clarifying:📝 Suggested comment fix
.breadcrumbs([ - // `@ts-expect-error` name is not a valid key of the customer object + // `@ts-expect-error` 'nam' is not a valid key of the customer object (typo of 'name') { param: 1, keys: ['id', 'nam'] },
Releases: @power-rent/try-catch@1.0.1 [skip ci]
|
1 participant
introduced GSD
fixed type safety issue in breadcrumbs
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.