feat(scripts): add timestamp update to check-connect-data #17015
Conversation
WalkthroughThe changes introduce a new function, ✨ Finishing Touches
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
scripts/ci/check-connect-data.ts (3)
38-47: Add input validation and optimize performance.The timestamp handling logic could be more robust and efficient:
- Add validation for timestamp format
- Handle invalid dates
- Optimize for large arrays by using a single iteration
Consider this improved implementation:
const getLatestTimestamp = (timestamps: string[]): string | null => { if (timestamps.length === 0) { return null; } - const dateObjects = timestamps.map(timestamp => new Date(timestamp)); - const latestDate = new Date(Math.max(...dateObjects.map(date => date.getTime()))); + let latestDate: Date | null = null; + for (const timestamp of timestamps) { + const date = new Date(timestamp); + if (isNaN(date.getTime())) { + console.warn(`Invalid timestamp format: ${timestamp}`); + continue; + } + if (!latestDate || date > latestDate) { + latestDate = date; + } + } - return latestDate.toISOString(); + return latestDate ? latestDate.toISOString() : null; };
76-81: Enhance error handling with more descriptive message.The error message could be more descriptive to aid in debugging.
Consider this improvement:
const latestTimestamp = getLatestTimestamp(timestamps); if (!latestTimestamp) { // Sanity check and type safety. - throw new Error('Timestamp should always be present.'); + console.error('Failed to determine latest timestamp from:', timestamps); + throw new Error('No valid timestamps found in authenticity data. This is unexpected as each authenticity file should contain a timestamp.'); }
83-83: Improve type safety for timestamp assignment.Consider using a constant for the timestamp key and ensuring type safety.
Consider these improvements:
// At the top of the file with other constants const TIMESTAMP_KEY = 'timestamp' as const; // Update the assignment deviceAuthenticityConfig[TIMESTAMP_KEY] = latestTimestamp;Also, ensure that
DeviceAuthenticityConfigtype (imported from './deviceAuthenticityConfigTypes') includes the timestamp property:interface DeviceAuthenticityConfig { [key: string]: { rootPubKeys: string[]; caPubKeys: string[]; debug: { rootPubKeys: string[]; caPubKeys: string[]; }; } | string; // Allow string for timestamp timestamp: string; // Explicitly define timestamp }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/ci/check-connect-data.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (1)
scripts/ci/check-connect-data.ts (1)
56-62: Verify if dev timestamps should be included.Currently, only timestamps from production authenticity data are being collected. Should timestamps from
authenticityDevdata also be considered?Please confirm if this is intentional or if we should also collect timestamps from dev data:
const timestamps: string[] = []; for (const deviceKey of devicesKeys) { const { authenticity, authenticityDev } = authenticityPaths[deviceKey]; const authenticityUrl = `${AUTHENTICITY_BASE_URL}/${authenticity}`; const authenticityData = await fetchJSON(authenticityUrl); timestamps.push(authenticityData.timestamp); + const authenticityDevUrl = `${AUTHENTICITY_BASE_URL}/${authenticityDev}`; + const authenticityDevData = await fetchJSON(authenticityDevUrl); + timestamps.push(authenticityDevData.timestamp);
scripts/ci/check-connect-data.ts
Outdated
| const dateObjects = timestamps.map(timestamp => new Date(timestamp)); | ||
| const latestDate = new Date(Math.max(...dateObjects.map(date => date.getTime()))); | ||
|
|
||
| return latestDate.toISOString(); |
There was a problem hiding this comment.
nitpick (no need for mapping the timestamps twice...):
| const dateObjects = timestamps.map(timestamp => new Date(timestamp)); | |
| const latestDate = new Date(Math.max(...dateObjects.map(date => date.getTime()))); | |
| return latestDate.toISOString(); | |
| const latestTime = Math.max(...timestamps.map(Date.parse)); | |
| return new Date(latestTime).toISOString(); | |
| }; |
karliatto
force-pushed
the
feat/check-connect-data-update-timestamp
branch
from
7389924 to
9d0d4d1
Compare
karliatto
added
the
no-project
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
scripts/ci/check-connect-data.ts (1)
175-175:⚠️ Potential issueFix template literal syntax error.
There's an extra backtick at the end of the error message template literal.
Apply this diff to fix the syntax error:
- console.error(`Failed to delete branch ${branchName}:`, error.message``); + console.error(`Failed to delete branch ${branchName}:`, error.message);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/ci/check-connect-data.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: check-project-or-issue
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (2)
scripts/ci/check-connect-data.ts (2)
38-46: Optimize timestamp calculation by combining map operations.The implementation is correct but can be optimized by combining the timestamp parsing and max calculation into a single operation.
Apply this diff to optimize the implementation:
const getLatestTimestamp = (timestamps: string[]): string | null => { if (timestamps.length === 0) { return null; } - const latestTime = Math.max(...timestamps.map(Date.parse)); - - return new Date(latestTime).toISOString(); + const latestTime = Math.max(...timestamps.map(Date.parse)); + return new Date(latestTime).toISOString(); };
75-83: LGTM! Good error handling for timestamp update.The implementation correctly handles the timestamp update with proper validation and error handling.
| const updateConfigFromJSON = async () => { | ||
| try { | ||
| const devicesKeys = Object.keys(authenticityPaths) as AuthenticityPathsKeys[]; | ||
|
|
||
| // Import the current configuration object | ||
| let { deviceAuthenticityConfig } = require(CONFIG_FILE_PATH); | ||
|
|
||
| const timestamps: string[] = []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for timestamp presence in authenticity data.
The code assumes that authenticityData.timestamp exists without validation. This could lead to runtime errors if the timestamp is undefined.
Apply this diff to add validation:
const timestamps: string[] = [];
for (const deviceKey of devicesKeys) {
const { authenticity, authenticityDev } = authenticityPaths[deviceKey];
const authenticityUrl = `${AUTHENTICITY_BASE_URL}/${authenticity}`;
const authenticityData = await fetchJSON(authenticityUrl);
- timestamps.push(authenticityData.timestamp);
+ if (!authenticityData.timestamp) {
+ throw new Error(`Missing timestamp in authenticity data for ${deviceKey}`);
+ }
+ timestamps.push(authenticityData.timestamp);Also applies to: 61-61
Description
Add timestamp update to check-connect-data