Conversation
|
👀 Reviewing the changes in PR #1025 now |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
LGTM — Clean implementation of a self-registration pattern for integration credential roles. The Proxy-backed backward compatibility layer correctly preserves all existing PROVIDER_CREDENTIAL_ROLES[provider] and PROVIDER_CATEGORY[provider] access patterns, Object.entries() / Object.keys() / Object.values() enumeration, and in operator behavior. The fix to hasPmIntegration correctly handles the semantic change from undefined to [] for unknown providers.
All 7 CI checks pass. The code is well-structured, properly documented, and the test coverage is solid.
Minor observations (not blocking):
-
Test cleanup registers rather than removes: The
afterEachin the new test block callsregisterCredentialRoles('test-provider-linear', 'pm', [])— this doesn't remove the provider from the registry, it keeps it registered with an empty role array. This is fine for the current test file since the concurrentPROVIDER_CREDENTIAL_ROLEStests run before the sequential registration tests, but if future tests in this file iterateObject.entries(PROVIDER_CREDENTIAL_ROLES), they'd see these ghost entries. Consider exposing anunregisterCredentialRolesfunction or a test-only_resetRegistry()helper if more tests are added later. -
Proxy
gettrap type annotation: Theget(_target, prop: string)signature narrowsproptostring, but the actual Proxy handler spec passesstring | symbol. This is harmless at runtime (Map lookup on a Symbol returnsundefined, and?? []handles it), but the type annotation is technically imprecise.
🕵️ claude-code · claude-opus-4-6 · run details
Summary
registerCredentialRoles(provider, category, roles)function tosrc/config/integrationRoles.tsso new providers can self-register at bootstrap time without hardcoded changesgetCredentialRoles(provider)accessor that returns roles for any registered provider (including dynamically registered ones), returning[]for unknown providersPROVIDER_CREDENTIAL_ROLESandPROVIDER_CATEGORYfrom plainconstobjects toProxy-backed accessors that read from an internal mutableMap— all existing call sites continue working unchangedhasPmIntegrationinsrc/pm/integration.tsto guard against the empty-array case (previously guarded againstundefined; now the Proxy returns[]for unknown providers)tests/unit/config/integrationRoles.test.tswith 8 new tests covering registration, retrieval, backward-compat proxy access, and overwrite behaviorTest plan
PROVIDER_CREDENTIAL_ROLESandPROVIDER_CATEGORYtests pass (backward compat)registerCredentialRolesadds a new provider,getCredentialRolesretrieves itPROVIDER_CREDENTIAL_ROLESandPROVIDER_CATEGORYhasPmIntegrationreturns false for unknown provider (empty-array guard fix)Card: https://trello.com/c/69c1b04df4b4bc3b4baf28e2
🤖 Generated with Claude Code
🕵️ claude-code · claude-sonnet-4-6 · run details