Skip to content

fix(automation): add Position Estimation to the Automation section nav#3359

Closed
Yeraze wants to merge 1 commit into
mainfrom
fix/automation-nav-position-estimation
Closed

fix(automation): add Position Estimation to the Automation section nav#3359
Yeraze wants to merge 1 commit into
mainfrom
fix/automation-nav-position-estimation

Conversation

@Yeraze
Copy link
Copy Markdown
Owner

@Yeraze Yeraze commented Jun 7, 2026

Problem

The Position Estimation settings section (from #3271 / #3349) is rendered in the Automation tab as <div id="position-estimation"> (App.tsx:5191), but it was never added to the tab's SectionNav quick-links list — which jumped straight from Auto Delete by Distance to Ignored Nodes. As a result the section had no quick-link and was only discoverable by scrolling, so users couldn't find where to configure the estimator (enable, frequency, lookback, "Recalculate now").

Fix

  • Add the missing { id: 'position-estimation', label: t('automation.position_estimation.title', 'Position Estimation') } entry to the Automation SectionNav, in render order (after Auto Delete by Distance, before Ignored Nodes).
  • Add AutomationSectionNav.test.ts — a static-source invariant guard (App.tsx is too large to render in jsdom) asserting:
    1. the Automation nav lists position-estimation, and
    2. every Automation nav item id has a matching <div id="..."> section anchor (catches drift in either direction).

Testing

  • New test: 2 passed.
  • tsc --noEmit clean.

🤖 Generated with Claude Code

The Position Estimation settings section (issue #3271/#3349) was rendered in
the Automation tab as <div id="position-estimation"> but was never added to
the SectionNav quick-links list, which jumped straight from "Auto Delete by
Distance" to "Ignored Nodes". The section was therefore only discoverable by
scrolling, and users reported being unable to find the feature's settings.

Add the missing nav entry (in render order) and a static-source invariant test
asserting every Automation nav item has a matching section anchor and that
Position Estimation is listed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 7, 2026

Claude finished @Yeraze's task —— View job


PR Review Complete

Todo List:

  • Read and analyze changed files
  • Review code quality and implementation
  • Assess test coverage and test quality
  • Check for potential bugs or issues
  • Evaluate performance and security considerations
  • Provide comprehensive review feedback

Code Review Feedback

This PR is well-executed and addresses a clear UX gap. Here's my assessment:

Positives

Problem Well-Defined: The issue description clearly identifies the problem - Position Estimation settings were missing from the navigation quick-links, making them hard to discover.

Minimal, Focused Change: The fix is surgical - just one line added to /src/App.tsx:5007 in the correct position (after Auto Delete by Distance, before Ignored Nodes).

Excellent Test Coverage: The new AutomationSectionNav.test.ts is particularly well-crafted:

  • Regression Prevention: Directly tests for the original bug (missing position-estimation nav item)
  • Future-Proof: Guards against similar issues by verifying ALL nav items have corresponding section anchors
  • Smart Approach: Uses static source analysis since App.tsx is too large for jsdom rendering
  • Clear Documentation: Excellent comments explaining the regression context

Follows Patterns: The navigation item follows the established pattern with proper i18n key structure.

📝 Code Quality Assessment

Implementation Quality: ⭐⭐⭐⭐⭐

  • Clean, consistent with existing code
  • Proper i18n usage with fallback text
  • Correctly positioned in logical order

Test Quality: ⭐⭐⭐⭐⭐

  • Comprehensive coverage of the specific issue
  • Future-proof design prevents similar regressions
  • Well-structured with clear intent
  • Good use of descriptive test names and assertions

🔍 Security & Performance

Security: ✅ No concerns

  • No security implications for a navigation link addition
  • Uses existing i18n infrastructure safely

Performance: ✅ No concerns

  • Single array item addition has negligible impact
  • Test uses efficient static file parsing

🎯 Potential Considerations

Minor: The test does static string parsing which could theoretically be brittle if App.tsx formatting changes significantly, but this is a reasonable tradeoff given App.tsx's size and the test's focused scope.

Note: I cannot run the test suite or linter to verify the reported passing tests, but the implementation looks solid.

🏆 Overall Assessment

This is a high-quality fix that:

  • ✅ Addresses the stated problem directly
  • ✅ Includes excellent regression prevention
  • ✅ Follows established patterns
  • ✅ Has minimal risk of introducing bugs
  • ✅ Improves user experience

Recommendation: Approve and merge - this is exactly the type of focused, well-tested fix that improves UX without risk.


@Yeraze
Copy link
Copy Markdown
Owner Author

Yeraze commented Jun 7, 2026

Superseded: rather than just adding a quick-link, Position Estimation is being relocated out of the per-source Automation tab into the global Settings UI. It's a global feature (single set of position_estimation_* keys, one cross-source batch job) and its backend endpoints are already gated by settings:read/settings:write — not automation. The move fixes both discoverability and the UI/permission mismatch. Closing in favor of that PR.

@Yeraze Yeraze closed this Jun 7, 2026
@Yeraze Yeraze deleted the fix/automation-nav-position-estimation branch June 7, 2026 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant