feat(byods): beta for BYoDS #4084
Conversation
Co-authored-by: RAM1232 <[email protected]>
…ebex#3870) Co-authored-by: parv_gour <parv_gour@PAGOUR-M-D8B2> Co-authored-by: rsarika <[email protected]>
Co-authored-by: Adhwaith Menon <[email protected]>
|
Note Reviews pausedUse the following commands to manage reviews:
📝 WalkthroughWalkthroughThe pull request implements a series of changes across multiple packages in the project. New workspace entries were added to the main configuration, integrating additional packages for a BYoDS demo server and a Node.js SDK. Several configuration files were introduced, including updated Babel, TypeScript, and Jest settings, which establish standardized build, testing, and runtime environments. New source files define classes and interfaces, such as for error handling, logging, HTTP clients, token management, and SDK functionality. Additionally, updated documentation and README files provide guidance on package usage and development instructions, while new HTML and CSS files establish user interface components for the demo server. Enhancements in the test suites further cover the functionality for each new module, and the legacy tools were adjusted to include additional unit test files from source directories. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@coderabbitai don't review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (44)
packages/byods/src/byods/index.ts (1)
128-134: Use a specific error class for better error handling.Currently, the method throws a generic
ErrorwhenorgIdis not provided. Consider using a custom error class to provide more context and enable better error handling.Apply this diff to use
ExtendedError:+ import ExtendedError from '../Errors/catalog/ExtendedError'; + import {ERROR_TYPE} from '../Errors/types'; if (!orgId) { - throw new Error(`orgId is required`); + throw new ExtendedError('orgId is required', ERROR_TYPE.VALIDATION_ERROR); }packages/byods/src/Logger/index.ts (1)
192-201: Include error stack trace in logs for improved debugging.Currently, the
errormethod logs only the error message but omits the stack trace. Including the stack trace can provide more context and aid in debugging.Apply this diff to include the stack trace:
this.writeToConsole( `${this.format(context, '[ERROR]')} - !${LOG_PREFIX.ERROR}!${LOG_PREFIX.MESSAGE}:${ - error.message + error.stack || error.message }`, LOGGER.ERROR );packages/byods/src/base-client/index.ts (2)
198-199: Implement handling for refresh token expirationThere is a TODO comment indicating that refresh token expiration needs to be handled. It's important to implement logic to manage situations when the refresh token has expired to prevent authentication issues.
Would you like assistance in implementing the logic for handling refresh token expiration?
78-88: Refactor to reduce code duplication in HTTP methodsThe
post,put, andpatchmethods share similar logic for making HTTP requests. Consider refactoring them to reduce code duplication, possibly by creating a helper method or adjusting therequestmethod to acceptbodyandheadersdirectly.Also applies to: 99-109, 120-130
packages/byods/src/token-manager/index.ts (3)
42-44: Use a more specific error type when required parameters are missingCurrently, when
clientIdorclientSecretis missing, a genericErroris thrown. Consider using a more specific error type or a custom error class to provide clearer context about the missing parameters.Apply this diff to improve the error handling:
if (!clientId || !clientSecret) { - throw new Error('clientId and clientSecret are required'); + throw new ExtendedError('clientId and clientSecret are required', ERROR_TYPE.INVALID_ARGUMENT); }
189-190: Consistent error handling inrefreshServiceAppAccessTokenWhen
orgIdis not provided, a genericErroris thrown. Consider using a consistent error handling approach by usingExtendedErrorto maintain uniformity across the codebase.Apply this diff for consistency:
if (!orgId) { - throw new Error('orgId not provided'); + throw new ExtendedError('orgId not provided', ERROR_TYPE.INVALID_ARGUMENT); }
154-176: Ensure original error details are included when rethrowingIn
getServiceAppTokenUsingPAT, when an error occurs, the original error is logged but only the error message is rethrown. To aid in debugging, consider including the original error when throwing.Apply this diff to include the original error:
throw error; + // or + throw new ExtendedError(error.message, error.type || ERROR_TYPE.TOKEN_ERROR, error);packages/byods/src/data-source-client/index.ts (2)
115-167: Simplify error handling inscheduleJWSTokenRefreshWhen rejecting promises in an
asyncfunction, usingthrowis more idiomatic thanreturn Promise.reject(error);. This ensures that the stack trace is preserved and the code remains clean.Apply this diff to simplify error handling:
} catch (error) { log.error( new ExtendedError( `Encountered error while trying to setup JWS refresh schedule ${error}`, ERROR_TYPE.SCHEDULE_JWS_TOKEN_REFRESH_ERROR ), { file: BYODS_DATA_SOURCE_CLIENT_MODULE, method: 'scheduleJWSTokenRefresh', } ); - return Promise.reject(error); + throw error; }
223-224: Simplify error rejection insaveServiceAppRegistrationDataWhen rejecting a promise in an
asyncfunction, usethrowinstead ofreturn Promise.reject(error);for better readability and stack trace preservation.Apply this diff to simplify error handling:
} catch (error) { log.error( new ExtendedError('Error saving service app registration', ERROR_TYPE.REGISTRATION_ERROR), {file: BYODS_TOKEN_MANAGER_MODULE, method: 'saveServiceAppRegistration'} ); - return Promise.reject(error); + throw error; }packages/byods/jest.setup.js (1)
1-12: Enhance the custom matcher implementation.While the implementation is functional, it could be more robust and provide better error messages.
Consider this improved implementation:
expect.extend({ toBeCalledOnceWith(received, ...expected) { + const {calls} = received.mock; + const pass = calls.length === 1 && this.equals(calls[0], expected); + + return { + pass, + message: () => { + if (calls.length !== 1) { + return `Expected mock to be called once but it was called ${calls.length} times`; + } + return pass + ? `Expected mock to not be called with ${this.utils.printExpected(expected)}` + : `Expected mock to be called with ${this.utils.printExpected(expected)}\n` + + `Received: ${this.utils.printReceived(calls[0])}`; + }, + }; - try { - expect(received).toBeCalledTimes(1); - expect(received).toBeCalledWith(...expected); - } catch (error) { - return {message: () => error, pass: false}; - } - - return {pass: true}; }, });This implementation:
- Provides more descriptive error messages
- Uses Jest's built-in utilities for better output formatting
- Directly checks the mock calls instead of relying on other matchers
packages/byods/babel.config.js (1)
7-10: Consider using a more specific Node.js version target.Setting
node: 'current'might lead to inconsistent builds across different environments.Consider specifying a minimum supported Node.js version:
targets: { - node: 'current', + node: '14.17.0', // or your minimum supported version },packages/byods/src/Errors/catalog/ExtendedError.ts (1)
3-5: Complete the empty JSDoc comment block.Add a description of the
ExtendedErrorclass purpose and usage./** + * Extended error class that adds error type categorization. + * Used for classifying errors throughout the BYoDS SDK. */packages/legacy/tools/src/models/package/package.constants.ts (1)
8-8: Consider using a more inclusive test pattern for BYODS.The current pattern
./*.test.tsonly matches test files in the root directory. This could miss test files in subdirectories.Consider using a more inclusive pattern:
- BYODS: './*.test.ts', + BYODS: './**/*.test.ts',packages/byods/src/Logger/types.ts (2)
9-9: Fix typo in LOG_PREFIX.MAIN value.There appears to be a typo in the main prefix value.
- MAIN = 'BYODOS_SDK', + MAIN = 'BYODS_SDK',
17-23: Consider adding JSDoc comments for LOGGING_LEVEL enum.The enum values are well-ordered, but documentation would help explain the significance of each level.
+/** + * Defines logging levels in ascending order of verbosity. + * Higher values indicate more detailed logging. + */ export enum LOGGING_LEVEL { + /** Critical errors that prevent normal operation */ error = 1, + /** Important issues that don't prevent operation */ warn = 2, + /** General application logs */ log = 3, + /** Detailed information for debugging */ info = 4, + /** Very detailed debugging information */ trace = 5, }packages/byods-demo-server/src/views/config.hbs (1)
13-23: Enhance form accessibility.The form could benefit from improved accessibility features.
- <form action="/config" method="post"> + <form action="/config" method="post" aria-labelledby="configFormTitle"> + <h2 id="configFormTitle">BYoDS Configuration</h2> <div> - <label for="clientId">Client ID:</label> + <label for="clientId" aria-required="true">Client ID:</label> <input type="text" id="clientId" name="clientId" required /> </div> <div> - <label for="clientSecret">Client Secret:</label> + <label for="clientSecret" aria-required="true">Client Secret:</label> <input type="password" id="clientSecret" name="clientSecret" required /> </div> - <button type="submit">Submit</button> + <button type="submit" aria-label="Save configuration">Submit</button> </form>packages/byods/src/constants.ts (3)
5-5: Consider using semantic versioning for beta releases.For beta releases, consider using version format like
1.0.0-beta.1to better indicate the pre-release status.-export const BYODS_SDK_VERSION = '0.0.1'; +export const BYODS_SDK_VERSION = '1.0.0-beta.1';
10-14: Consider environment configuration for URLs.The base URLs and JWKS URLs are hardcoded. Consider making these configurable through environment variables or configuration objects for better flexibility across different environments.
-export const PRODUCTION_BASE_URL = 'https://webexapis.com/v1'; -export const INTEGRATION_BASE_URL = 'https://integration.webexapis.com/v1'; -export const PRODUCTION_JWKS_URL = 'https://idbroker.webex.com/idb/oauth2/v2/keys/verificationjwk'; -export const INTEGRATION_JWKS_URL = - 'https://idbrokerbts.webex.com/idb/oauth2/v2/keys/verificationjwk'; +export const BASE_URLS = { + production: process.env.BYODS_PRODUCTION_BASE_URL || 'https://webexapis.com/v1', + integration: process.env.BYODS_INTEGRATION_BASE_URL || 'https://integration.webexapis.com/v1' +}; +export const JWKS_URLS = { + production: process.env.BYODS_PRODUCTION_JWKS_URL || 'https://idbroker.webex.com/idb/oauth2/v2/keys/verificationjwk', + integration: process.env.BYODS_INTEGRATION_JWKS_URL || 'https://idbrokerbts.webex.com/idb/oauth2/v2/keys/verificationjwk' +};
20-20: Consider more verbose logging for beta release.The default logger level is set to ERROR which might be too restrictive for a beta release where more verbose logging could help with debugging and feedback.
-export const DEFAULT_LOGGER_CONFIG = {level: LOGGER.ERROR}; +export const DEFAULT_LOGGER_CONFIG = { + level: process.env.NODE_ENV === 'production' ? LOGGER.ERROR : LOGGER.DEBUG +};packages/byods/src/Errors/types.ts (1)
18-28: Consider adding additional HTTP status codes.The ERROR_CODE enum is missing some common HTTP status codes that might be useful:
- 201 (Created) for successful resource creation
- 204 (No Content) for successful deletion
- 409 (Conflict) for resource conflicts
- 502 (Bad Gateway) for upstream service issues
export enum ERROR_CODE { UNAUTHORIZED = 401, FORBIDDEN = 403, DEVICE_NOT_FOUND = 404, INTERNAL_SERVER_ERROR = 500, NOT_IMPLEMENTED = 501, SERVICE_UNAVAILABLE = 503, BAD_REQUEST = 400, REQUEST_TIMEOUT = 408, TOO_MANY_REQUESTS = 429, + CREATED = 201, + NO_CONTENT = 204, + CONFLICT = 409, + BAD_GATEWAY = 502, }packages/byods/jest.config.js (1)
15-22: Consider relaxing coverage thresholds for beta.The current thresholds of 85% might be too strict for a beta release. Consider temporarily lowering them and gradually increasing as the codebase matures.
coverageThreshold: { global: { - lines: 85, - functions: 85, - branches: 85, - statements: 85, + lines: 70, + functions: 70, + branches: 60, + statements: 70, }, },packages/byods/src/token-storage-adapter/types.ts (2)
16-16: Enhance method documentation with error handling.The interface methods should document potential errors that might be thrown. This is especially important for a beta release where implementers need clear guidance.
/** * Method to set the token for an organization. * @param orgId Organization ID * @param token Respective token + * @throws {Error} If token is invalid or storage fails * @example * await storageAdapter.setToken('org-id', token); */ setToken(orgId: string, token: OrgServiceAppAuthorization): Promise<void>; /** * Method to extract a token id based on the organization id. * @param orgId Organization * @returns {OrgServiceAppAuthorization} The token object for the organization + * @throws {Error} If token is not found or storage access fails * @example * const token = await storageAdapter.getToken('org-id'); */ getToken(orgId: string): Promise<OrgServiceAppAuthorization>;Also applies to: 25-25, 33-33, 42-42, 50-50
8-51: Consider adding validation requirements.The interface should specify validation requirements for the
orgIdparameter and token structure. This will help ensure consistent implementation across different storage adapters.Add validation requirements to the interface documentation:
/** * Represents a adapter for storing and retrieving service app authorization tokens. * + * Implementation requirements: + * - orgId must be a non-empty string + * - token must be a valid OrgServiceAppAuthorization object + * - Storage operations should be atomic + * - Implementations should handle concurrent access * @public */ export interface TokenStorageAdapter {packages/byods/src/http-client/types.ts (1)
21-59: Consider using stricter types for request bodies.Instead of using
Record<string, any>for request bodies, consider using generic type parameters or specific interfaces to improve type safety.Example improvement:
- post<T>(endpoint: string, body: Record<string, any>): Promise<ApiResponse<T>>; + post<T, B = unknown>(endpoint: string, body: B): Promise<ApiResponse<T>>;Apply similar changes to
putandpatchmethods.packages/byods/src/token-storage-adapter/index.ts (1)
9-77: LGTM! Well-implemented token storage adapter.The implementation is clean, type-safe, and includes proper error handling.
Consider adding token validation and expiration checks.
To enhance reliability, consider:
- Validating token structure before storage
- Checking token expiration during retrieval
Example enhancement:
async getToken(orgId: string): Promise<OrgServiceAppAuthorization> { if (this.tokenCache[orgId]) { + const token = this.tokenCache[orgId]; + if (new Date() >= token.serviceAppToken.expiresAt) { + throw new Error(`Token expired for org ID: ${orgId}`); + } - return this.tokenCache[orgId]; + return token; } throw new Error(`Service App token not found for org ID: ${orgId}`); }packages/byods/test/unit/spec/Logger/index.ts (2)
23-38: Improve test description and assertions.The test description could be clearer, and the assertions use magic numbers.
Suggested improvements:
- it('Set the log level to error and verify that all levels are not executed except error', () => { + it('should only log errors when log level is ERROR', () => { log.info(fakePrint, dummyContext); - expect(logSpy).not.toHaveBeenCalledTimes(1); + expect(logSpy).not.toHaveBeenCalled(); log.log(fakePrint, dummyContext); - expect(logSpy).not.toHaveBeenCalledTimes(1); + expect(logSpy).not.toHaveBeenCalled();
36-37: Verify error message content.The test should verify the content of the error message, not just that it was called.
log.error(new Error(fakePrint) as ExtendedError, dummyContext); - expect(errorSpy).toHaveBeenCalledTimes(1); + expect(errorSpy).toHaveBeenCalledWith( + expect.stringContaining(fakePrint), + expect.objectContaining(dummyContext) + );packages/byods/src/types.ts (1)
42-67: Consider using string literal type for token_type.The
token_typefield could be more strictly typed to prevent invalid values.- token_type: string; + token_type: 'Bearer' | 'JWT';packages/byods/src/data-source-client/types.ts (3)
77-77: Add URL format validation.Consider adding a URL format validation using a regex pattern or URL constructor to ensure valid URLs are provided.
- url: string; + url: string & { __brand: 'URL' }; + + // Add a type guard function + function isValidUrl(url: string): url is string & { __brand: 'URL' } { + try { + new URL(url); + return true; + } catch { + return false; + } + }
97-97: Add validation for token lifetime.Consider adding a reasonable range for token lifetime to prevent issues with extremely short or long durations.
- tokenLifetimeMinutes: number; + tokenLifetimeMinutes: number & { __brand: 'TokenLifetime' }; + + // Add a type guard function + function isValidTokenLifetime(minutes: number): minutes is number & { __brand: 'TokenLifetime' } { + return minutes >= 5 && minutes <= 1440; // Between 5 minutes and 24 hours + }
117-119: Add JSDoc documentation for Cancellable interface.The interface lacks documentation explaining its purpose and usage.
+/** + * Represents a cancellable operation. + * + * @public + */ export interface Cancellable { + /** + * Cancels the ongoing operation. + */ cancel: () => void; }packages/byods/test/unit/spec/token-storage-adapter/index.ts (2)
12-14: Use Jest's clock mocking for date handling.Replace hardcoded dates with Jest's fake timers to make tests more reliable and maintainable.
+ beforeAll(() => { + jest.useFakeTimers(); + jest.setSystemTime(new Date('2025-02-15T10:00:00Z')); + }); + + afterAll(() => { + jest.useRealTimers(); + }); - expiresAt: new Date(Date.now() + 3600 * 1000), - refreshAccessTokenExpiresAt: new Date(Date.now() + 7200 * 1000), + expiresAt: new Date('2025-02-15T11:00:00Z'), // 1 hour from now + refreshAccessTokenExpiresAt: new Date('2025-02-15T12:00:00Z'), // 2 hours from now
65-73: Extract test data setup to helper function.Reduce code duplication by extracting token creation logic to a helper function.
+ function createTestToken(orgId: string): OrgServiceAppAuthorization { + return { + orgId, + serviceAppToken: { + accessToken: `access-token-${orgId}`, + refreshToken: `refresh-token-${orgId}`, + expiresAt: new Date('2025-02-15T11:00:00Z'), + refreshAccessTokenExpiresAt: new Date('2025-02-15T12:00:00Z'), + }, + }; + } - const dummyToken: OrgServiceAppAuthorization = { - orgId: dummyOrgId, - serviceAppToken: { - accessToken: 'another-access-token', - refreshToken: 'another-refresh-token', - expiresAt: new Date(Date.now() + 3600 * 1000), - refreshAccessTokenExpiresAt: new Date(Date.now() + 7200 * 1000), - }, - }; + const dummyToken = createTestToken(dummyOrgId);packages/byods/test/unit/spec/base-client/index.ts (1)
8-82: Add test cases for error handling and edge cases.The test suite covers the happy path scenarios well, but would benefit from additional test cases:
- Error handling for network failures
- Edge cases with invalid/empty parameters
- Token manager error scenarios
- Request retry logic (if implemented)
Here's an example of additional test cases to consider:
it('should handle network errors', async () => { const mockError = new Error('Network failure'); jest.spyOn(baseClient, 'request').mockRejectedValue(mockError); await expect(baseClient.get('/test-endpoint')).rejects.toThrow('Network failure'); }); it('should handle invalid parameters', async () => { await expect(baseClient.get('')).rejects.toThrow(); await expect(baseClient.post('/endpoint', undefined)).rejects.toThrow(); }); it('should handle token manager errors', async () => { jest.spyOn(baseClient.tokenManager, 'getToken').mockRejectedValue(new Error('Token error')); await expect(baseClient.get('/test-endpoint')).rejects.toThrow('Token error'); });packages/byods/test/unit/spec/byods/index.ts (2)
24-29: Use more realistic mock data.The mock SDK configuration uses placeholder values. Consider using realistic-looking values to make tests more representative of actual usage.
const mockSDKConfig: SDKConfig = { clientId: 'byods-client-12345', clientSecret: 'sec-98765-abcdef-ghijk', tokenStorageAdapter: new InMemoryTokenStorageAdapter(), logger: { level: LOGGER.ERROR, format: 'json' }, baseUrl: 'https://api.example.com/v1' };
55-91: Enhance JWS token validation test cases.The JWS token validation tests could be more comprehensive. Consider adding tests for:
- Malformed tokens
- Invalid signatures
- Missing claims
- Token replay attacks
it('should handle malformed JWS tokens', async () => { const malformedJws = 'invalid.token.format'; (jwtVerify as jest.Mock).mockRejectedValueOnce(new Error('Invalid token format')); const result = await sdk.verifyJWSToken(malformedJws); expect(result).toEqual({ isValid: false, error: 'Invalid token format' }); }); it('should handle missing required claims', async () => { const jwsWithMissingClaims = 'valid.format.token'; (jwtVerify as jest.Mock).mockResolvedValue({ payload: {}, // Missing required claims protectedHeader: { alg: 'HS256' } }); const result = await sdk.verifyJWSToken(jwsWithMissingClaims); expect(result).toEqual({ isValid: false, error: 'Missing required claims' }); });packages/legacy/tools/src/models/package/package.ts (2)
103-109: Make test pattern configuration more flexible.The test pattern is hardcoded to use
CONSTANTS.PATTERNS.BYODS. Consider making this configurable through the test configuration.interface TestConfig { unit?: boolean; integration?: boolean; runner?: string; + srcTestPattern?: string; } const unitTestFileCollectorInSrc = config.unit ? Package.getFiles({ location: path.join(this.data.packageRoot, CONSTANTS.TEST_DIRECTORIES.SRC), - pattern: CONSTANTS.PATTERNS.BYODS, + pattern: config.srcTestPattern || CONSTANTS.PATTERNS.BYODS, targets: config.targets, }) : Promise.resolve([]);
127-131: Optimize test file collection.The current implementation collects and concatenates test files separately. Consider collecting them in a single operation for better performance.
const collectTestFiles = async (config: TestConfig) => { const patterns = []; if (config.unit) { patterns.push( path.join(CONSTANTS.TEST_DIRECTORIES.UNIT, CONSTANTS.PATTERNS.TEST), path.join(CONSTANTS.TEST_DIRECTORIES.SRC, config.srcTestPattern || CONSTANTS.PATTERNS.BYODS) ); } return Package.getFiles({ location: this.data.packageRoot, patterns, targets: config.targets }); };packages/byods/test/unit/spec/token-manager/index.ts (1)
1-238: Enhance test coverage with additional test cases.Consider adding the following test cases to improve coverage:
- Edge cases for token expiry (e.g., token about to expire).
- Concurrent token refresh scenarios to ensure thread safety.
- Refresh token expiry handling.
Would you like me to help generate these additional test cases?
packages/byods/test/unit/spec/data-source-client/index.ts (1)
171-185: Add test cases for additional error scenarios.Consider adding test cases for:
- Network timeouts
- Rate limiting responses
- Invalid response formats
Would you like me to help generate these additional test cases?
packages/byods-demo-server/src/views/data-source.hbs (1)
73-94: Add client-side form validation.The form lacks comprehensive client-side validation. Consider:
- Adding input pattern validation
- Implementing real-time validation feedback
- Adding length constraints for inputs
Would you like me to help implement these validation improvements?
packages/byods/README.md (3)
1-1: Fix Node.js spelling.Update the spelling to follow the official Node.js style guide.
-### Bring Your Own Data Source (BYoDS) Node.JS SDK +### Bring Your Own Data Source (BYoDS) Node.js SDK🧰 Tools
🪛 LanguageTool
[uncategorized] ~1-~1: The official spelling of this programming framework is “Node.js”.
Context: ### Bring Your Own Data Source (BYoDS) Node.JS SDK ### Table of Contents - [Getting ...(NODE_JS)
17-17: Add hyphenation and enhance feature description.The sentence needs hyphenation and could benefit from more detailed feature descriptions.
-The BYoDS Node.js SDK makes it easy for developers to register their data sources to the BYoDS system. It allows developers to build data sources without needing to manage the complexities of integration. With features like customizable storage, service app token management, and auto refresh of JWS tokens. The BYoDS SDK provides a solid foundation for creating secure and reliable data sources. +The BYoDS Node.js SDK makes it easy for developers to register their data sources to the BYoDS system. It allows developers to build data sources without needing to manage the complexities of integration. With features like customizable storage, service app token management, and auto-refresh of JWS tokens, the BYoDS SDK provides a solid foundation for creating secure and reliable data sources.🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...rage, service app token management, and auto refresh of JWS tokens. The BYoDS SDK provides a...(AUTO_HYPHEN)
15-66: Enhance documentation with additional sections.Consider adding:
- API reference documentation
- Configuration options
- Troubleshooting guide
- Security best practices
- Example use cases
Would you like me to help expand the documentation with these sections?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...rage, service app token management, and auto refresh of JWS tokens. The BYoDS SDK provides a...(AUTO_HYPHEN)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (48)
package.json(1 hunks)packages/byods-demo-server/.gitignore(1 hunks)packages/byods-demo-server/README.md(1 hunks)packages/byods-demo-server/babel.config.js(1 hunks)packages/byods-demo-server/package.json(1 hunks)packages/byods-demo-server/src/index.ts(1 hunks)packages/byods-demo-server/src/public/stylesheets/config.css(1 hunks)packages/byods-demo-server/src/public/stylesheets/data-source.css(1 hunks)packages/byods-demo-server/src/public/stylesheets/orgs.css(1 hunks)packages/byods-demo-server/src/server.ts(1 hunks)packages/byods-demo-server/src/views/config.hbs(1 hunks)packages/byods-demo-server/src/views/data-source.hbs(1 hunks)packages/byods-demo-server/src/views/orgs.hbs(1 hunks)packages/byods-demo-server/tsconfig.json(1 hunks)packages/byods/.gitignore(1 hunks)packages/byods/README.md(1 hunks)packages/byods/babel.config.js(1 hunks)packages/byods/jest.config.js(1 hunks)packages/byods/jest.d.ts(1 hunks)packages/byods/jest.setup.js(1 hunks)packages/byods/package.json(1 hunks)packages/byods/src/Errors/catalog/ExtendedError.ts(1 hunks)packages/byods/src/Errors/types.ts(1 hunks)packages/byods/src/Logger/index.ts(1 hunks)packages/byods/src/Logger/types.ts(1 hunks)packages/byods/src/base-client/index.ts(1 hunks)packages/byods/src/byods/index.ts(1 hunks)packages/byods/src/constants.ts(1 hunks)packages/byods/src/data-source-client/constants.ts(1 hunks)packages/byods/src/data-source-client/index.ts(1 hunks)packages/byods/src/data-source-client/types.ts(1 hunks)packages/byods/src/http-client/types.ts(1 hunks)packages/byods/src/http-utils/index.ts(1 hunks)packages/byods/src/index.ts(1 hunks)packages/byods/src/token-manager/index.ts(1 hunks)packages/byods/src/token-storage-adapter/index.ts(1 hunks)packages/byods/src/token-storage-adapter/types.ts(1 hunks)packages/byods/src/types.ts(1 hunks)packages/byods/test/unit/spec/Logger/index.ts(1 hunks)packages/byods/test/unit/spec/base-client/index.ts(1 hunks)packages/byods/test/unit/spec/byods/index.ts(1 hunks)packages/byods/test/unit/spec/data-source-client/index.ts(1 hunks)packages/byods/test/unit/spec/http-utils/index.ts(1 hunks)packages/byods/test/unit/spec/token-manager/index.ts(1 hunks)packages/byods/test/unit/spec/token-storage-adapter/index.ts(1 hunks)packages/byods/tsconfig.json(1 hunks)packages/legacy/tools/src/models/package/package.constants.ts(2 hunks)packages/legacy/tools/src/models/package/package.ts(2 hunks)
✅ Files skipped from review due to trivial changes (8)
- packages/byods-demo-server/.gitignore
- packages/byods/src/data-source-client/constants.ts
- packages/byods/tsconfig.json
- packages/byods-demo-server/src/index.ts
- packages/byods-demo-server/README.md
- packages/byods/.gitignore
- packages/byods-demo-server/tsconfig.json
- packages/byods-demo-server/babel.config.js
🧰 Additional context used
🪛 Biome (1.9.4)
packages/byods/test/unit/spec/data-source-client/index.ts
[error] 297-297: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🪛 LanguageTool
packages/byods/README.md
[uncategorized] ~1-~1: The official spelling of this programming framework is “Node.js”.
Context: ### Bring Your Own Data Source (BYoDS) Node.JS SDK ### Table of Contents - [Getting ...
(NODE_JS)
[uncategorized] ~17-~17: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...rage, service app token management, and auto refresh of JWS tokens. The BYoDS SDK provides a...
(AUTO_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Initialize Project
🔇 Additional comments (14)
packages/byods/src/byods/index.ts (1)
103-118: Verification logic is implemented correctly.The
verifyJWSTokenmethod correctly verifies the JWS token using the public key and handles errors appropriately.packages/byods/jest.d.ts (1)
1-10: LGTM! Well-structured type declaration.The custom matcher declaration follows TypeScript best practices and properly extends Jest's typing system.
packages/byods/babel.config.js (1)
14-14: Verify if private methods transform is necessary.TypeScript already handles private methods transformation. The
@babel/plugin-transform-private-methodsplugin might be redundant.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if private methods are used in the codebase rg -l "private\s+\w+\s*\(" .Length of output: 2953
Babel Private Methods Plugin is Likely Redundant
The shell script confirms that many TypeScript files already use theprivatemodifier, and TypeScript’s compiler transforms these as needed. In the byods Babel configuration, applying@babel/plugin-transform-private-methodsappears unnecessary if the TypeScript compilation step is handling the transformation.
- Location:
packages/byods/babel.config.js(line 14)- Action: Confirm that the byods package is solely compiled via TypeScript and that all environments are adequately supported by its output. If so, consider removing the plugin.
packages/byods/src/index.ts (1)
1-9: LGTM! Well-structured barrel file.The file follows the barrel pattern correctly to expose the public API of the BYoDS SDK. The imports and exports are properly organized.
packages/byods/jest.config.js (1)
13-14: Enable coverage collection for beta testing.The TODO comment indicates pending test files. For beta release, it's important to track coverage to ensure quality. Consider enabling coverage collection now.
-// TODO: Set this to true once we have the source code and their corresponding test files added -collectCoverage: false, +collectCoverage: true,✅ Verification successful
Enable coverage collection for beta testing.
The update in the Jest configuration (packages/byods/jest.config.js) switches the coverage flag from false to true, which will help ensure that code quality is monitored during the beta phase.
- Updated the coverage configuration by setting
collectCoverage: trueand removing the TODO comment.packages/byods/src/http-client/types.ts (1)
6-16: LGTM! Well-designed generic response type.The
ApiResponse<T>interface is well-documented and provides a flexible, type-safe way to handle API responses.packages/byods/src/types.ts (1)
74-94: Consider adding date validation for token expiration.The
ServiceAppTokeninterface could benefit from runtime validation to ensure valid dates.Consider adding a validation function:
function isValidServiceAppToken(token: ServiceAppToken): boolean { const now = new Date(); return ( token.expiresAt instanceof Date && token.refreshAccessTokenExpiresAt instanceof Date && token.expiresAt > now && token.refreshAccessTokenExpiresAt > now ); }packages/byods/src/http-utils/index.ts (1)
21-22: Address the TODO comment regarding node-fetch issue.The TODO comment references an issue with node-fetch. Consider implementing a workaround or upgrading to a newer version where this issue is fixed.
✅ Verification successful
🌐 Web query:
Is node-fetch issue #1809 resolved in the latest version?💡 Result:
The provided search results do not contain any explicit mention of **node-fetch issue #1809**. However, based on the latest available information from the search results (as of February 2025): 1. **Latest node-fetch version** referenced is **3.3.2** (released July 25, 2023) [8], though newer versions may exist beyond the scope of these search results. 2. The GitHub releases page ([8]) shows bug fixes up to **v3.3.2**, including header handling and premature close errors, but no direct reference to issue #1809. To verify resolution: - Check [node-fetch's GitHub Issues](https://github.com/node-fetch/node-fetch/issues/1809) directly (not included in provided sources). - If resolved, it would appear in release notes for versions after **3.3.2**. - If unresolved, temporary workarounds might include: ```javascript // For CommonJS users stuck with older versions const fetch = require('node-fetch@2'); ``` Or migrating to Node.js v21+ with native Fetch API support [5]. For authoritative confirmation, consult the official repository's closed issues or changelogs beyond what's shown in these search results. Citations: - 1: https://ahmadrosid.com/blog/fix-node-fetch-import - 2: https://dev.to/logrocket/the-fetch-api-is-finally-coming-to-nodejs-foe?comments_sort=latest - 3: https://www.codingdeft.com/posts/nodejs-referenceerror-fetch-is-not-defined/ - 4: https://learn.microsoft.com/en-us/answers/questions/740430/cant-get-node-fetch-to-work-(my-first-azure-functi - 5: https://blog.logrocket.com/fetch-api-node-js/ - 6: https://www.npmjs.com/package/node-fetch-native - 7: https://community.freshworks.dev/t/issues-with-node-fetch/8285 - 8: https://github.com/node-fetch/node-fetch/releases - 9: https://codedamn.com/news/nodejs/fetch-is-not-defined - 10: https://community.dynatrace.com/t5/Developer-Q-A-Forum/Get-error-fetch-failed-when-i-make-a-post-request-on-localhost/m-p/205151
Node-fetch Issue #1809 Still Unresolved – Consider a Workaround or Migration
Based on the latest web search results, node-fetch issue #1809 does not appear to have been explicitly resolved in the current releases (up to v3.3.2). This means the TODO comment in
packages/byods/src/http-utils/index.tsremains valid. You might consider one of the following options:
- Migrate to Native Fetch: If you’re running on Node.js v21+ (which has native fetch support), you can eliminate the dependency on node-fetch.
- Temporary Workaround: For environments that require node-fetch, consider using an alternative version (e.g., node-fetch@2 for CommonJS support) until a proper fix is released.
packages/byods-demo-server/package.json (1)
1-56: Solid configuration for the BYoDS Demo Server.
The package configuration is well-organized—defining the project’s name, version, privacy, scripts (for both normal and hot-reload development), Node engine requirements, and clear separation between runtime and development dependencies. The workspace setup (via the added entries in the root package.json) effectively prepares the project for multi-package management.packages/byods-demo-server/src/public/stylesheets/orgs.css (1)
1-136: Comprehensive and cohesive styling for organizational components.
The CSS rules here are clearly segmented:
- Global body and heading styles set a clean, modern tone.
- Classes like
.main-containerand.orgs-containeruse flexbox and spacing to center and visually separate content.- The button styles include gradient backgrounds and smooth hover transitions, adding a nice interactive feel.
Make sure to verify that the chosen fonts, colors, and spacing meet both brand and accessibility guidelines.
packages/byods-demo-server/src/public/stylesheets/config.css (1)
1-145: Robust theming and responsive design with config.css.
This file is well-structured and leverages modern CSS practices:
- It imports a web font and defines a comprehensive set of CSS variables that simplify future theming and adjustments.
- The reset rules, flexible body layout, and detailed styling for headers, main content, and forms collectively establish a consistent, maintainable design.
- The inclusion of media queries ensures that the interface is responsive across devices.
No issues found—just ensure the values align with the broader design system.
packages/byods/package.json (1)
1-124: Well-defined package configuration for @webex/byods.
The file clearly specifies:
- Package metadata (name, main entry point, types, license, author, and repository information),
- Engine requirements enforcing Node.js ≥20.x,
- A robust set of scripts that cover building, testing (both unit and style checks), and deployment tasks, and
- Commitlint rules to promote consistent commit messaging.
This comprehensive setup supports both development and quality assurance.
packages/byods-demo-server/src/public/stylesheets/data-source.css (1)
1-361: Detailed and expressive CSS for data-source components.
This extensive CSS file defines the presentation for various UI elements used in data visualization and management, including:
- Global resets and base styles for the body and headings,
- Clearly defined container classes (e.g.,
.main-container,.data-container,.data-sources-list),- Card-like components with hover effects (
.data-source-card), ensuring an interactive and modern look,- Button group styles and specific controls (icon buttons, refresh/verify buttons) that emphasize consistency, and
- Modal and form element styling that enhances user interaction and usability.
All components appear well-considered and should contribute to an engaging user interface. Make sure to test hover, focus, and transition effects across multiple browsers for cross-compatibility.
package.json (1)
13-14: New BYoDS Workspace Entries Added
The addition of./packages/byodsand./packages/byods-demo-servercorrectly expands the project’s workspaces to include the BYoDS-related packages. Please ensure that these new directories each contain a properly configuredpackage.jsonand that any workspace-specific scripts or dependencies are updated accordingly.
packages/byods/src/byods/index.ts
Outdated
| const parsedEnv = process.env.BYODS_ENVIRONMENT || 'production'; | ||
| let jwksUrl = PRODUCTION_BASE_URL; | ||
|
|
There was a problem hiding this comment.
Correct the initial jwksUrl assignment to use the JWKS URL.
In the constructor, jwksUrl is initially assigned PRODUCTION_BASE_URL, which is incorrect. It should be assigned PRODUCTION_JWKS_URL to correctly point to the JWKS endpoint.
Apply this diff to fix the assignment:
- let jwksUrl = PRODUCTION_BASE_URL;
+ let jwksUrl = PRODUCTION_JWKS_URL;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const parsedEnv = process.env.BYODS_ENVIRONMENT || 'production'; | |
| let jwksUrl = PRODUCTION_BASE_URL; | |
| const parsedEnv = process.env.BYODS_ENVIRONMENT || 'production'; | |
| let jwksUrl = PRODUCTION_JWKS_URL; |
| let sdk: BYODS.BYODS; | ||
| let baseClient: BYODS.BaseClient; | ||
|
|
There was a problem hiding this comment.
Avoid using global variables for sdk and baseClient.
Using global variables for sdk and baseClient can lead to concurrency issues in a server environment. If multiple requests modify these variables simultaneously, it may cause unexpected behavior. Consider initializing these objects within the scope of each request or managing their state using middleware.
Refactor the code to instantiate sdk and baseClient within request handlers:
-let sdk: BYODS.BYODS;
-let baseClient: BYODS.BaseClient;
...
-app.post('/config', (req: Request, res: Response) => {
+app.post('/config', (req: Request, res: Response) => {
+ let sdk: BYODS.BYODS;
const {clientId, clientSecret} = req.body;
const config = {
clientId,
clientSecret,
tokenStorageAdapter: new BYODS.InMemoryTokenStorageAdapter(),
logger: {level: BYODS.LOGGER.LOG},
};
sdk = new BYODS.BYODS(config);
res.redirect('/orgs');
});
...
-app.get('/data-source', async (req: Request, res: Response) => {
+app.get('/data-source', async (req: Request, res: Response) => {
+ let baseClient: BYODS.BaseClient;
const {orgId} = req.query;
baseClient = sdk.getClientForOrg(orgId as string);
try {
const dataSourcesResponse = await baseClient.dataSource.list();
const dataSources = dataSourcesResponse.data;
renderTemplate(res, 'data-source', {title: 'Data Sources', orgId, dataSources});
} catch (error) {
res.status(400).send(`Error fetching data sources: ${error.message}`);
}
});Committable suggestion skipped: line range outside the PR's diff.
| const {tokenLifetimeMinutes} = req.body; | ||
| await baseClient.dataSource.scheduleJWSTokenRefresh(id, tokenLifetimeMinutes, () => | ||
| crypto.randomUUID() | ||
| ); // Use UUID for nonce |
There was a problem hiding this comment.
Import the crypto module to use crypto.randomUUID().
The function crypto.randomUUID() is used but the crypto module is not imported, which will result in a runtime error.
Apply this diff to import the crypto module:
+import crypto from 'crypto';
...
await baseClient.dataSource.scheduleJWSTokenRefresh(id, tokenLifetimeMinutes, () =>
crypto.randomUUID()
); // Use UUID for nonce📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const {tokenLifetimeMinutes} = req.body; | |
| await baseClient.dataSource.scheduleJWSTokenRefresh(id, tokenLifetimeMinutes, () => | |
| crypto.randomUUID() | |
| ); // Use UUID for nonce | |
| import crypto from 'crypto'; | |
| const {tokenLifetimeMinutes} = req.body; | |
| await baseClient.dataSource.scheduleJWSTokenRefresh(id, tokenLifetimeMinutes, () => | |
| crypto.randomUUID() | |
| ); // Use UUID for nonce |
| Authorization: `Bearer ${token}`, | ||
| ...this.headers, | ||
| ...options.headers, | ||
| }, |
There was a problem hiding this comment.
Ensure critical headers are not overwritten unintentionally
In the request method, the header merging logic allows options.headers to potentially overwrite critical headers like Authorization. To prevent accidental overwriting, consider adjusting the order of header merging.
Apply this diff to adjust the header merging order:
headers: {
- Authorization: `Bearer ${token}`,
- ...this.headers,
- ...options.headers,
+ ...this.headers,
+ ...options.headers,
+ Authorization: `Bearer ${token}`,
},This ensures that the Authorization header is not overridden by headers in this.headers or options.headers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Authorization: `Bearer ${token}`, | |
| ...this.headers, | |
| ...options.headers, | |
| }, | |
| headers: { | |
| ...this.headers, | |
| ...options.headers, | |
| Authorization: `Bearer ${token}`, | |
| }, |
| private async getToken(): Promise<string> { | ||
| const serviceAppAuthorization = await this.tokenManager.getOrgServiceAppAuthorization( | ||
| this.orgId | ||
| ); | ||
| let token = serviceAppAuthorization.serviceAppToken.accessToken; | ||
|
|
||
| if (new Date() >= new Date(serviceAppAuthorization.serviceAppToken.expiresAt)) { | ||
| await this.tokenManager.refreshServiceAppAccessToken(this.orgId, this.headers); | ||
| const refreshedAuthorization = await this.tokenManager.getOrgServiceAppAuthorization( | ||
| this.orgId | ||
| ); | ||
| token = refreshedAuthorization.serviceAppToken.accessToken; | ||
| } | ||
| // TODO: Handle refresh token expiration | ||
|
|
||
| return token; | ||
| } |
There was a problem hiding this comment.
Add error handling in the getToken method
Currently, the getToken method does not handle potential errors from this.tokenManager.getOrgServiceAppAuthorization or this.tokenManager.refreshServiceAppAccessToken. If these methods throw an error, getToken will fail without providing meaningful error information. Consider adding error handling to catch exceptions and log errors appropriately.
Apply this diff to add error handling:
+ try {
const serviceAppAuthorization = await this.tokenManager.getOrgServiceAppAuthorization(
this.orgId
);
let token = serviceAppAuthorization.serviceAppToken.accessToken;
if (new Date() >= new Date(serviceAppAuthorization.serviceAppToken.expiresAt)) {
await this.tokenManager.refreshServiceAppAccessToken(this.orgId, this.headers);
const refreshedAuthorization = await this.tokenManager.getOrgServiceAppAuthorization(
this.orgId
);
token = refreshedAuthorization.serviceAppToken.accessToken;
}
// TODO: Handle refresh token expiration
return token;
+ } catch (error) {
+ log.error('Error obtaining token', { error });
+ throw new Error('Failed to obtain token');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async getToken(): Promise<string> { | |
| const serviceAppAuthorization = await this.tokenManager.getOrgServiceAppAuthorization( | |
| this.orgId | |
| ); | |
| let token = serviceAppAuthorization.serviceAppToken.accessToken; | |
| if (new Date() >= new Date(serviceAppAuthorization.serviceAppToken.expiresAt)) { | |
| await this.tokenManager.refreshServiceAppAccessToken(this.orgId, this.headers); | |
| const refreshedAuthorization = await this.tokenManager.getOrgServiceAppAuthorization( | |
| this.orgId | |
| ); | |
| token = refreshedAuthorization.serviceAppToken.accessToken; | |
| } | |
| // TODO: Handle refresh token expiration | |
| return token; | |
| } | |
| private async getToken(): Promise<string> { | |
| try { | |
| const serviceAppAuthorization = await this.tokenManager.getOrgServiceAppAuthorization( | |
| this.orgId | |
| ); | |
| let token = serviceAppAuthorization.serviceAppToken.accessToken; | |
| if (new Date() >= new Date(serviceAppAuthorization.serviceAppToken.expiresAt)) { | |
| await this.tokenManager.refreshServiceAppAccessToken(this.orgId, this.headers); | |
| const refreshedAuthorization = await this.tokenManager.getOrgServiceAppAuthorization( | |
| this.orgId | |
| ); | |
| token = refreshedAuthorization.serviceAppToken.accessToken; | |
| } | |
| // TODO: Handle refresh token expiration | |
| return token; | |
| } catch (error) { | |
| log.error('Error obtaining token', { error }); | |
| throw new Error('Failed to obtain token'); | |
| } | |
| } |
| if (!response.ok) { | ||
| throw new Error(`HTTP Error Response: ${response.status} ${response.statusText}`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with specific error types.
The current error handling is basic. Consider implementing specific error types for different HTTP status codes and scenarios.
class HttpError extends Error {
constructor(public status: number, public statusText: string) {
super(`HTTP Error Response: ${status} ${statusText}`);
this.name = 'HttpError';
}
}
// In the request function:
if (!response.ok) {
throw new HttpError(response.status, response.statusText);
}| async function request<T>(url: string, options: HttpRequestInit = {}): Promise<ApiResponse<T>> { | ||
| // TODO: Fix this issue (which is being tracked in node_fetch) https://github.com/node-fetch/node-fetch/issues/1809 | ||
| const fetch = (await import('node-fetch')).default; | ||
| const optionsWithHeaders = { | ||
| ...options, | ||
| headers: { | ||
| Trackingid: `${BYODS_PACKAGE_NAME}_${randomUUID()}`, | ||
| 'User-Agent': USER_AGENT, | ||
| ...options.headers, | ||
| }, | ||
| }; | ||
|
|
||
| const response = await fetch(url, optionsWithHeaders); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`HTTP Error Response: ${response.status} ${response.statusText}`); | ||
| } | ||
|
|
||
| // Handle 204 No Content responses | ||
| if (response.status === 204) { | ||
| return {data: {} as T, status: response.status}; | ||
| } | ||
|
|
||
| const data = (await response.json()) as T; | ||
|
|
||
| return {data, status: response.status}; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add request timeout configuration.
The request function lacks timeout configuration, which could lead to hanging requests in production.
interface HttpRequestInit {
body?: string | null;
headers?: Record<string, string>;
method?: string;
+ timeout?: number;
}
async function request<T>(url: string, options: HttpRequestInit = {}): Promise<ApiResponse<T>> {
const fetch = (await import('node-fetch')).default;
const optionsWithHeaders = {
...options,
+ timeout: options.timeout || 30000, // Default 30s timeout
headers: {
Trackingid: `${BYODS_PACKAGE_NAME}_${randomUUID()}`,
'User-Agent': USER_AGENT,
...options.headers,
},
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function request<T>(url: string, options: HttpRequestInit = {}): Promise<ApiResponse<T>> { | |
| // TODO: Fix this issue (which is being tracked in node_fetch) https://github.com/node-fetch/node-fetch/issues/1809 | |
| const fetch = (await import('node-fetch')).default; | |
| const optionsWithHeaders = { | |
| ...options, | |
| headers: { | |
| Trackingid: `${BYODS_PACKAGE_NAME}_${randomUUID()}`, | |
| 'User-Agent': USER_AGENT, | |
| ...options.headers, | |
| }, | |
| }; | |
| const response = await fetch(url, optionsWithHeaders); | |
| if (!response.ok) { | |
| throw new Error(`HTTP Error Response: ${response.status} ${response.statusText}`); | |
| } | |
| // Handle 204 No Content responses | |
| if (response.status === 204) { | |
| return {data: {} as T, status: response.status}; | |
| } | |
| const data = (await response.json()) as T; | |
| return {data, status: response.status}; | |
| } | |
| interface HttpRequestInit { | |
| body?: string | null; | |
| headers?: Record<string, string>; | |
| method?: string; | |
| timeout?: number; | |
| } | |
| async function request<T>(url: string, options: HttpRequestInit = {}): Promise<ApiResponse<T>> { | |
| // TODO: Fix this issue (which is being tracked in node_fetch) https://github.com/node-fetch/node-fetch/issues/1809 | |
| const fetch = (await import('node-fetch')).default; | |
| const optionsWithHeaders = { | |
| ...options, | |
| timeout: options.timeout || 30000, // Default 30s timeout | |
| headers: { | |
| Trackingid: `${BYODS_PACKAGE_NAME}_${randomUUID()}`, | |
| 'User-Agent': USER_AGENT, | |
| ...options.headers, | |
| }, | |
| }; | |
| const response = await fetch(url, optionsWithHeaders); | |
| if (!response.ok) { | |
| throw new Error(`HTTP Error Response: ${response.status} ${response.statusText}`); | |
| } | |
| // Handle 204 No Content responses | |
| if (response.status === 204) { | |
| return {data: {} as T, status: response.status}; | |
| } | |
| const data = (await response.json()) as T; | |
| return {data, status: response.status}; | |
| } |
| sub: 'mySubject', | ||
| aud: 'myAudience', | ||
| }); | ||
| jest.spyOn(global, 'setInterval').mockImplementation((fn: Function) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace Function type with explicit function signature.
The Function type is too broad and can lead to type-safety issues.
Apply this diff to improve type safety:
-jest.spyOn(global, 'setInterval').mockImplementation((fn: Function) => {
+jest.spyOn(global, 'setInterval').mockImplementation((fn: () => void) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jest.spyOn(global, 'setInterval').mockImplementation((fn: Function) => { | |
| jest.spyOn(global, 'setInterval').mockImplementation((fn: () => void) => { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 297-297: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
| function decodeJWSToken(jwsToken) { | ||
| return JSON.parse(atob(jwsToken.split('.')[1])); | ||
| } |
There was a problem hiding this comment.
Move token decoding to server-side.
Client-side token decoding using atob is insecure as it exposes token contents. Consider moving this logic to the server.
| function submitForm(endpoint, payload) { | ||
| fetch(endpoint, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json' | ||
| }, | ||
| body: JSON.stringify(payload) | ||
| }) | ||
| .then(response => response.json()) | ||
| .then(data => { | ||
| if (data.error) { | ||
| throw new Error(data.error); | ||
| } | ||
| alert(`Data source ${isEdit ? 'updated' : 'added'} successfully`); | ||
| location.reload(); | ||
| }) | ||
| .catch(error => { | ||
| alert(`Error ${isEdit ? 'updating' : 'adding'} data source: ${error.message}`); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in form submission.
The current error handling is basic. Consider:
- Adding specific error types and messages
- Implementing retry logic for transient failures
- Preserving form data on failure
|
@bhabalan: I'll pause the automated review for this PR. ✅ Actions performedReviews paused. |
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
Coverage report |
COMPLETES #< SPARK-614847 >
This pull request addresses
Merge BYoDS SDK to next
by making the following changes
cherry-pick all commit from feat/byods into next
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.