refactor: remove the duplication and clarity findings from the review - #365
Merged
Merged
Conversation
Four places restated something the code already knew. The repository's "does this row carry telemetry" filter listed seventy fields by hand, next to merge loops that read the same fields off the model by reflection. A field added to the model was silently treated as empty until someone extended that list. Both filters are now built from a type: the general one from the model, the chart one from TelemetryHistoryPoint, whose field list it was copying. The API and the Worker each configured Serilog and the tyre pressure thresholds with identical blocks. They now share GarageStack.Core.Configuration. HostingExtensions, and the thresholds fall back per value, so a deployment that passes an empty string gets the app's defaults rather than zeroes. Three Worker services read db.Vehicles directly, past IVehicleRepository. They go through the repository now; the one write that made this awkward, the parked-at timestamp, has a repository method rather than a tracked entity save. The frontend detected the drivetrain by parsing hw_version itself, duplicating VehicleTypeHelper. The vehicle list endpoint now reports the detected type. Also fixes a flaky test: the command gate's hold window was measured with DateTime.UtcNow, whose ~15ms tick can read a full wait back as slightly short. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A dashboard card was spread over a defaults list, an icon map, a has-data switch and the renderer's own config table, which repeated the icons and the data checks a third time. src/cards/registry.ts now describes each card once: icon, default visibility per drivetrain, and whether the telemetry holds anything for it. The card ids, the default layout, the skeleton, the edit-mode placeholders and the render guards all read from it, so a new card is one entry plus its markup. usePoiLayers repeated the same state and loading logic three times, once per layer, with the fetch-id guard, tile bookkeeping and cluster handling copied each time. That engine now lives in composables/poiTileLayer.ts, and charging stations, fuel stations and service areas are three configurations of it, each with its own fetch, filter and marker. Layers that do not apply to the vehicle (a plug for an HEV, a tank for a BEV) fold into the layer's "enabled" instead of a watch apiece. Components looked the car up as vehicles[0] in nine places; the store now exposes activeVehicle/activeVin, and the drivetrain comes from the API rather than from parsing hw_version in the browser. StatsChartCard took bar data cast as line data. It now takes the chart kind as a prop and renders through vue-chartjs's own Chart component, so neither side lies. MapView's style block was entirely unscoped for the sake of the Leaflet markers in it; only those stay global now. Stylelint runs over the stylesheets, which is how the duplicate rule in main.css and the duplicated flex-shrink turned up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tings
The api and worker services repeated their whole environment block, and both took
env_file: .env, which handed every container every secret in the file. The shared
half is a YAML anchor now, and each service lists what it gets: the Worker no
longer receives the OIDC client secret, the widget key or the MG credentials, and
the API no longer receives the notification language. Compose reads .env for
${...} interpolation either way, so nothing needs the blanket passthrough.
Versions that sat in several files now sit in one. pnpm lives in package.json's
packageManager field, which corepack and pnpm/action-setup both read, replacing
four pins in CI and two in Dockerfiles. The release workflow installs the same
MinVer the build uses, read from Directory.Packages.props: it was pinned to
minver-cli 7.0.0 while the package had moved to 8.0.0, so the image tag and the
version stamped inside it could come from different majors.
Runtime defaults belong to the app, so the compose files and the all-in-one
entrypoint pass a setting through only when the operator sets one. The tyre
pressure thresholds and the rate limit no longer restate the app's own numbers.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The auth store cached its config request for the life of the page, including a failed one. If the API was not up yet, the login page showed "No sign-in method is available" and stayed that way: no form, no retry, nothing but a reload. A container restart was enough to trigger it, which is how it surfaced. A failed attempt is no longer cached, the page retries a few times before giving up, and the message it does show says the API could not be reached and offers a retry. "No sign-in method" now means what it says: the API answered and has nothing configured. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Pull Request
Summary
Works through the duplication and clarity list from the review. Nothing here changes what the app does, with two exceptions called out below.
Frontend
DashboardCardContent's own config table, which repeated the icons and data checks again.src/cards/registry.tsnow holds one entry per card (icon, default visibility per drivetrain, whether the telemetry has anything to show), and the card ids, default layout, skeleton, edit-mode placeholders and render guards are all derived from it.usePoiLayerscarried the same fetch-id guard, tile bookkeeping, cache and cluster handling three times. That engine is nowcomposables/poiTileLayer.ts, and the three layers are configurations of it. "This layer does not apply to this vehicle" folds into the layer's enabled state rather than a watch each.vehicles[0]in nine components becomesstore.activeVehicle/store.activeVin.ChartData<'line'>. It takes the chart kind as a prop and renders through vue-chartjs's ownChart, so the cast is gone.pnpm lintand the lint job), which is what found the duplicate:rootand the duplicatedflex-shrink.Backend
HasDatalisted seventy fields by hand, beside merge loops that read the same fields off the model reflectively, so a new telemetry field was treated as empty until someone extended the list. Both filters are built from a type now, the chart one fromTelemetryHistoryPoint, whose field list it was copying.Core.Configuration.HostingExtensions.db.VehiclespastIVehicleRepository. The one write that made that awkward, the parked-at timestamp, has a repository method.hw_versionitself, duplicatingVehicleTypeHelper. The vehicle list endpoint reports the detected drivetrain.Deployment
env_file: .envhanded every container every secret. The shared half is a YAML anchor; each service lists what it gets. The Worker no longer receives the OIDC secret, the widget key or the MG credentials.packageManager, which corepack andpnpm/action-setupboth read (was four pins in CI, two in Dockerfiles). The release workflow installs the MinVer version fromDirectory.Packages.props: it was pinned tominver-cli 7.0.0while the package had moved to8.0.0, so the image tag and the version stamped inside it could come from different majors.Two behaviour changes
remainingChargeandspeedcould render an empty card (a charge timer on an HEV, speed before any is reported). The registry's predicates cover those cases, so the slot is dropped instead.Type of Change
Related Issues
Follow-up to the review findings raised after #364.
Checklist
pnpm lint/dotnet build)auth.apiUnreachable,common.retry, both languages)Verification
dotnet formatclean, no pending model changes.🤖 Generated with Claude Code