chore: capture current state and polish GitHub surface - #79
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 95 |
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
Retain both repository histories and resolve the integration to the reviewed polished tree.
There was a problem hiding this comment.
Pull Request Overview
While the project remains 'Up to Standards' according to Codacy, this PR introduces significant architectural changes that lack verification. Specifically, the extraction of complex physics logic into systemStateDirectKepler.ts (Complexity Delta: 78) without new unit tests represents a high regression risk. The PR is labeled as a 'chore,' yet it performs deep refactoring of simulation internals and UI synchronization logic.
There is a notable gap in the project hygiene: RELEASE_STATUS.md identifies .impeccable/design.json as a blocker for hygiene checks, but this PR fails to add that file to .gitignore or untrack it. Furthermore, documentation indicates that macOS and Python-based tests were bypassed, meaning the 'captured state' of the project includes known verification debt.
Logic-level issues include redundant orbital calculations in the emission time solver and inefficient UI event listeners that trigger double-sync cycles. These should be addressed to maintain performance during interactive simulation runs.
About this PR
- The
RELEASE_STATUS.mdfile notes that hygiene checks are blocked by the tracked.impeccable/design.jsonfile, yet this file was not added to.gitignoreor untracked in this PR. Please ensure this file is handled to unblock automated hygiene checks. - This PR is labeled as a 'chore' but contains substantial architectural refactoring of the simulation core. Given the complexity of the changes in
systemStateDirectKepler.ts, this should be treated as a feature or refactor PR with mandatory unit test coverage for the extracted physics logic.
Test suggestions
- Verify scenario control range extraction and DOM application
- Verify direct Kepler system state calculations (refactored logic in systemStateDirectKepler.ts)
- Verify UI input synchronization via quick control bindings (refactored logic in quickControls.ts)
- Confirm margin adjustment in responsive.css resolves the reported 8px horizontal overflow
- Unit tests for resolveDirectEmissionTime and enhancedDirectShapiroConfig with relativity configurations
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify direct Kepler system state calculations (refactored logic in systemStateDirectKepler.ts)
2. Verify UI input synchronization via quick control bindings (refactored logic in quickControls.ts)
3. Confirm margin adjustment in responsive.css resolves the reported 8px horizontal overflow
4. Unit tests for resolveDirectEmissionTime and enhancedDirectShapiroConfig with relativity configurations
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| @@ -0,0 +1,489 @@ | |||
| /** Resolves direct Kepler system states, including exomoon timing and relativity corrections. */ | |||
There was a problem hiding this comment.
🔴 HIGH RISK
This new module centralizes high-complexity orbital physics (Complexity: 78) extracted from the previous system state. The move currently lacks unit tests, which is critical given the precision-sensitive nature of emission time and Shapiro delay calculations. Please add unit tests covering resolveDirectEmissionTime and enhancedDirectShapiroConfig.
| rel, | ||
| shapiroSolve, | ||
| rAtTime: (time) => | ||
| sampler.pairStateAt(time)?.moon?.r ?? |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The multiple calls to sampler.pairStateAt(time) should be coalesced into a single variable to avoid redundant orbital resolutions in the emission time solver. Additionally, use the baryState property from the resolved pair to avoid the final fallback call to sampler.baryStateAt.
| return range ? [id, range] : undefined; | ||
| } | ||
|
|
||
| const scenarioControls = ((scenarioJson as { ui?: { controls?: ScenarioControl[] } }).ui?.controls ?? []) |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Refactoring the logic to use explicit map and filter steps with the scenarioControlRangeEntry helper would improve readability and simplify testing.
const scenarioControls = ((scenarioJson as { ui?: { controls?: ScenarioControl[] } }).ui?.controls ?? [])
.map(scenarioControlRangeEntry)
.filter((entry): entry is readonly [string, ScenarioNumericControlRange] => entry !== undefined);| listenerOptions: AddEventListenerOptions | undefined, | ||
| ): void { | ||
| for (const control of rawControls) { | ||
| control.addEventListener("input", syncFromRaw, listenerOptions); |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Remove one of the redundant event listeners in wireRawQuickSynchronizers or update dispatchInputAndChange to avoid triggering multiple UI sync cycles for the same modification. Currently, both 'input' and 'change' events trigger syncFromRaw.
Summary
Verification
git diff --checkReview note
This is intentionally a broad state-capture PR. Review the commits separately: product-state capture first, repository polish second.