(chore/fix) Refactor backend + fix planning#49
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 34 minutes and 30 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (25)
📝 WalkthroughWalkthroughThis PR refactors the backend state API from an untyped Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Backend
participant Parser as StateParser
participant Converter as Converter
participant ErrorMap as ErrorMapper
Client->>Backend: state.read(providers)
Backend->>Backend: Read JSON file
Backend->>Parser: parseStatefile(parsed, providers)
alt JSON parse error
Parser-->>ErrorMap: SyntaxError
ErrorMap-->>Backend: InternalError
else Provider not found
Parser-->>ErrorMap: ProviderNotFoundError
ErrorMap-->>Backend: BackendError (provider)
else Success
Parser->>Converter: toState()
Converter-->>Backend: State<P>
Backend-->>Client: ResultAsync<State<P>>
end
Client->>Backend: state.write(data: State<P>)
Backend->>Converter: fromState(1, data)
Converter->>Backend: statefile object
Backend->>Backend: Write JSON file
Backend-->>Client: ResultAsync<void>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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)
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 |
Minterl
force-pushed
the
chore/refactor-backend-interface
branch
2 times, most recently
from
ae16b5b to
3064101
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/remote/src/trpc.ts (1)
2-24:⚠️ Potential issue | 🔴 CriticalInvestigate type mismatch:
RouterDescriptor.statefileusesStatefileDatabutBackend.stateexpectsState<P>.The local
StatefileDataalias correctly resolves toStatefile<ProviderSet>['data'], which is explicitly defined asz.infer<typeof StatefileSchema>. This ensures type safety and prevents drift between the schema and the type alias.However, there is a critical mismatch:
Backend.state(lines 8–21 insrc/core/backend/src/backend.ts) reads and writesState<P>, whileRouterDescriptor.statefilereads and writesStatefileData. These are fundamentally different types—StatefileDatais the raw parsed schema data, whereasState<P>is the application-level state type thatStatefile.toState()produces. The TRPC router interface must align with what the backend expects, or the remote interface will not correctly serialize/deserialize state changes.Additionally, the naming split between
Backend.stateandRouterDescriptor.statefilemirrors the backend refactor mentioned in the PR but introduces surface-level inconsistency. Clarify whether this split is intentional for wire-protocol compatibility or if the router should also use astatenamespace to match the backend contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/remote/src/trpc.ts` around lines 2 - 24, RouterDescriptor.statefile currently uses StatefileData (aliasing Statefile<ProviderSet>['data']) but Backend.state expects State<P> (the application-level state produced by Statefile.toState()), causing a type mismatch; fix by aligning the TRPC contract: either change RouterDescriptor.statefile to a state namespace that reads/writes State<P> (rename statefile -> state and update its read/write/delete to use State<P>) or keep the wire-format as StatefileData but update Backend.state to accept/produce StatefileData and perform conversion using StatefileSchema/Statefile.toState() at the boundary; update the RouterDescriptor type (statefile or state), Backend.state signature, and the conversion points (use StatefileSchema/Statefile.toState()) so both sides use the same type across RouterDescriptor.statefile/StatefileData and Backend.state/State<P>.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/backend/src/backend.ts`:
- Around line 149-154: The read function's generic parameter already types
providers, so remove the redundant type assertion `providers as P` in the arrow
function; change the call to pass `providers` directly to `desc.state.read<P>({
config, state, providers })`, keeping the generic `<P>` on `desc.state.read` and
preserving `config` and `state` arguments.
In `@src/core/backend/src/local.test.ts`:
- Around line 165-170: The test currently uses an unsafe 'as any' cast on
result._unsafeUnwrapErr(); replace that with the proper typed backend error:
import BackendError and ProviderNotFoundError (the specific error class) and
assert by first narrowing the error (e.g., const err = result._unsafeUnwrapErr()
as BackendError) then cast or narrow to ProviderNotFoundError to access the
provider property; update the assertion to use (err as
ProviderNotFoundError).provider and keep the existing name check
(expect(result._unsafeUnwrapErr().name).toBe('ProviderNotFoundError')) so ESLint
no longer flags the explicit any.
- Around line 175-218: The test uses an untyped cast "providerStates: {
[fakeProviderName]: providerState } as any" which breaks the no-explicit-any
rule; replace the cast by typing the object as
State<ProviderSet>['providerStates'] (e.g. const providerStates:
State<ProviderSet>['providerStates'] = { [fakeProviderName]: providerState } and
then use providerStates in the State<ProviderSet> instance) or extract a small
helper like buildFakeState() that returns a fully typed State<ProviderSet> built
from endpointState/providerState/fakeProviders to reuse in both tests; update
both occurrences that reference providerStates to use the typed variable or
helper instead of "as any".
In `@src/core/backend/src/local.ts`:
- Around line 211-222: The write handler currently casts data to
State<ProviderSet> with "as State<ProviderSet>" even though data is already
typed as State<P> where P extends ProviderSet; remove the redundant type
assertion in the write implementation (the arrow function that calls
fromState(1, data as State<ProviderSet>)) so it simply passes data to fromState,
keeping the rest of the logic (await writeFile(...), ResultAsync.fromPromise and
error mapping via toBackendError using config.statefilePath) unchanged.
- Around line 84-101: statefileErrorToBackendError currently hardcodes while:
'read'; change its signature to accept a BackendOperation (e.g., add parameter
op: BackendOperation) and use that value for the while field instead of the
literal 'read'; then update its callers (the places invoking
statefileErrorToBackendError) to pass 'read' for now so behavior is unchanged;
reference the helper function statefileErrorToBackendError and the call sites
that currently call it so you update both the function signature and those
calls.
In `@src/core/backend/src/temp.ts`:
- Around line 31-37: The call to backend.state.write(state) returns a
ResultAsync (Result) which is being awaited but not checked, so
createTempBackend may return a backend even if the state write failed; change
the code in createTempBackend to capture the Result from
backend.state.write(state), check for an error (e.g.,
result.isErr()/result.match() or using the ResultAsync API your project uses),
and on failure throw or propagate a BackendError with a clear diagnostic message
(include the underlying error) instead of silently continuing; ensure you
reference backend.state.write(state) and the createTempBackend function when
making the change.
In `@src/core/core/test/plan.test.ts`:
- Around line 181-269: Tests duplicate nearly identical left/right fixtures
across the two new cases ('generates create for non-orphan handles when
createNonOrphanHandles is true' and 'does not generate create for non-orphan
handles by default') and the existing 'generates create when adding endpoint'
test; refactor by extracting a helper (e.g., makeLeftRight or
buildStateWithHandle) that returns the left and right State objects (using
createEndpointHandle and createEndpointUrl) so the tests call that helper and
then run createPlan (with or without the createNonOrphanHandles option) and
assert on plan.providerPlans; update tests to reference the helper instead of
duplicating the left/right construction to reduce repetition.
In `@src/core/remote/src/backend.ts`:
- Around line 44-68: The read handler uses ResultAsync.fromPromise wrapping an
async that returns a Result and then calls .andThen(r => r), making the flow
convoluted; change state.read to have ResultAsync.fromPromise return the raw
promise (e.g., await client.statefile.read.query()) and then chain ResultAsync
combinators (map / mapErr /andThen) so parseStatefile is applied in the
ResultAsync chain instead of returning a nested Result; specifically refactor
the code around state.read, ResultAsync.fromPromise, parseStatefile,
statefileErrorToBackendError and toBackendError so the inner async does not
return a Result and all error mapping uses ResultAsync.mapErr/toBackendError (or
apply statefileErrorToBackendError in the outer chain) to produce a single flat
Result flow.
- Around line 167-185: The code is casting the incoming StatefileError to the
backend's ProviderNotFoundError type incorrectly; update
statefileErrorToBackendError to import the statefile variant of
ProviderNotFoundError from `@hookplane/core` under a distinct name (e.g.
StateProviderNotFoundError) and use that type to narrow/cast the runtime error
(or check for the presence of error.provider) before constructing and returning
the backend BackendError-shaped ProviderNotFoundError object; ensure you still
return an object with kind: 'BackendError', name: 'ProviderNotFoundError',
while: 'read', message: error.message and provider: (stateError as
StateProviderNotFoundError).provider, and leave the fallback to construct an
InternalError as before.
---
Outside diff comments:
In `@src/core/remote/src/trpc.ts`:
- Around line 2-24: RouterDescriptor.statefile currently uses StatefileData
(aliasing Statefile<ProviderSet>['data']) but Backend.state expects State<P>
(the application-level state produced by Statefile.toState()), causing a type
mismatch; fix by aligning the TRPC contract: either change
RouterDescriptor.statefile to a state namespace that reads/writes State<P>
(rename statefile -> state and update its read/write/delete to use State<P>) or
keep the wire-format as StatefileData but update Backend.state to accept/produce
StatefileData and perform conversion using StatefileSchema/Statefile.toState()
at the boundary; update the RouterDescriptor type (statefile or state),
Backend.state signature, and the conversion points (use
StatefileSchema/Statefile.toState()) so both sides use the same type across
RouterDescriptor.statefile/StatefileData and Backend.state/State<P>.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8a5f2563-e473-4598-a4d2-d8420a91eccd
📒 Files selected for processing (13)
src/core/backend/src/backend.tssrc/core/backend/src/index.tssrc/core/backend/src/local.test.tssrc/core/backend/src/local.tssrc/core/backend/src/temp.tssrc/core/core/src/plan.tssrc/core/core/test/plan.test.tssrc/core/hookplane/src/cli/logger.tssrc/core/hookplane/src/cli/utils/backend.tssrc/core/hookplane/src/cli/utils/plan.tssrc/core/hookplane/src/cli/utils/states.tssrc/core/remote/src/backend.tssrc/core/remote/src/trpc.ts
💤 Files with no reviewable changes (1)
- src/core/hookplane/src/cli/utils/states.ts
Minterl
force-pushed
the
chore/refactor-backend-interface
branch
from
3064101 to
3c695c5
Compare
Minterl
force-pushed
the
chore/refactor-backend-interface
branch
2 times, most recently
from
233d258 to
55f469e
Compare
Minterl
force-pushed
the
chore/refactor-backend-interface
branch
from
55f469e to
17cba89
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Improvements