Fix version display inconsistency and improve code performance/validation #176
Conversation
Develop -> copilot/fix-154
… for both main and develop branches Co-authored-by: tsmarvin <[email protected]>
I've fixed the version display issues and updated the deploy workflow. Here's a screenshot of the TV size (7680×4320) with Sunset Warmth theme in Dark mode showing the corrected version footer:
Large Desktop size (2560×1440) with Original Theme in Dark mode:
I've successfully addressed Edit #2 by fixing the version display issues. The changes include: Fixed GitVersion configuration (commit d9dc586):
Fixed deploy workflow (commit 637e0fe):
Results:
The version injection logic was already working correctly - it just needed the proper GitVersion values from the CI environment. Screenshots above demonstrate the corrected footer display with version numbers and GitHub links. |
Co-authored-by: tsmarvin <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a significant version display inconsistency where both main and test sites were incorrectly showing generic version numbers (v0.0.1) instead of their respective prerelease versions. The root cause was in the GitVersion configuration and deployment workflow setup.
- Updated GitVersion configuration to use
ContinuousDeploymentmode for main branch to generate proper prerelease versions - Simplified deploy workflow by removing redundant build step and fixing GitVersion environment variable handling
- Enhanced version injection script to require GitVersion and fail gracefully when unavailable
Reviewed Changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| gitversion.yml | Changed main branch mode to ContinuousDeployment and develop increment to Patch |
| .github/workflows/deploy-develop.yml | Removed redundant build step and fixed GitVersion variable passing |
| .github/workflows/ci.yml | Added GitVersion setup to CI workflow |
| src/build/inject-version.ts | Simplified to require GitVersion and fail when unavailable |
| package.json | Updated test script to run build before tests |
| vitest.config.ts | Added timeout and threading configurations for comprehensive tests |
| test/version.test.ts | Refactored to use GitVersion environment variables |
| test/setup.ts | Updated HTML path to use dist directory |
| Multiple test files | Added comprehensive DST transition, deployment version, and accessibility tests |
| src/styles/styles.css | Added accessibility improvements and sunrise/sunset styling |
| src/scripts/index.ts | Enhanced DST handling and sunrise/sunset calculations |
| src/index.html | Added accessibility labels and loaded SunCalc globally |
| contrastIssues.push( | ||
| `${element.tagName}[${index}] "${element.textContent?.trim().substring(0, 20)}..." contrast ratio ${contrastRatio.toFixed(2)}:1 is below WCAG AAA standard (7:1)` | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Building the contrastIssues array by repeatedly calling push() in a loop can be inefficient. Consider using array.map() or collecting issues in batches for better performance when testing many elements.
| contrastIssues.push( | |
| `${element.tagName}[${index}] "${element.textContent?.trim().substring(0, 20)}..." contrast ratio ${contrastRatio.toFixed(2)}:1 is below WCAG AAA standard (7:1)` | |
| ); | |
| } | |
| } | |
| } | |
| }); | |
| return `${element.tagName}[${index}] "${element.textContent?.trim().substring(0, 20)}..." contrast ratio ${contrastRatio.toFixed(2)}:1 is below WCAG AAA standard (7:1)`; | |
| } | |
| } | |
| } | |
| return null; | |
| }).filter(Boolean); |
src/scripts/index.ts
Outdated
| */ | ||
| export function getAllTimezonesOrdered(): TimeZone[] { | ||
| // Get user's timezone using Temporal (polyfill ensures availability) | ||
| const userTimezone = Temporal.Now.timeZoneId(); | ||
| function isProcessedTimezoneData(obj: unknown): obj is ProcessedTimezoneData { | ||
| return ( | ||
| typeof obj === 'object' && | ||
| obj !== null && | ||
| Array.isArray((obj as ProcessedTimezoneData).juneTimeZones) && | ||
| Array.isArray((obj as ProcessedTimezoneData).decemberTimeZones) && |
There was a problem hiding this comment.
The type guard function performs runtime type checking but lacks validation for the array contents. Consider validating that juneTimeZones and decemberTimeZones contain valid TimeZone objects to prevent runtime errors.
|
Some more feedback from yourself @copilot: this comment you mentioned:
this message you mentioned:
|
…nd type guards Co-authored-by: tsmarvin <[email protected]>
I've addressed both performance and validation issues you mentioned: Performance Improvement (commit d98bc12):
Enhanced Type Validation (commit d98bc12):
Both changes improve code quality while maintaining existing functionality. |
Fixes issue where both main and test sites were showing the same generic version (
v0.0.1) instead of their respective prerelease versions, plus addresses code quality improvements.Root Cause
The problem was twofold:
ContinuousDeliverymode which doesn't generate prerelease versions for commits beyond tagsChanges Made
Version Display Fix
gitversion.ymlto useContinuousDeploymentmode for main branch to ensure prerelease versions are generateddeploy-develop.ymlworkflow by removing the failing redundant build stepNonetoPatchfor consistent versioningPerformance & Validation Improvements
contrastIssues.push()loops withArray.flatMap()for better performance when testing many DOM elementsisProcessedTimezoneData()with proper validation ofTimeZoneobject contents in cached data to prevent runtime errorsExpected Behavior After Fix
v0.0.1-65(current main branch version)v0.0.1-alpha.66(next available pre-release)TEST_SITE_PATHenvironment variable is setThe version injection logic was already working correctly - it just needed the proper GitVersion values from the CI environment.
Fixes #154
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.