Skip to content

Comments

Remove trailing slash from traceBaseUrl#479

Merged
lowrt merged 1 commit intomainfrom
fix/url
Dec 4, 2025
Merged

Remove trailing slash from traceBaseUrl#479
lowrt merged 1 commit intomainfrom
fix/url

Conversation

@lowrt
Copy link
Contributor

@lowrt lowrt commented Dec 4, 2025

這是什麼類型的 PR?

選擇所有符合的項目

  • 重構
  • 新功能
  • 錯誤修復
  • 最佳化
  • 技術文件更新

描述

(crashlytics)
修正結尾網址

相關 issue

  • 相關問題 #
  • closes #

QA 指南、截圖、錄像

請將這行替換成:如何測試您的 PR 的步驟,已測試的裝置註釋,以及任何相關的 UI 更改圖片。

UI 無障礙清單

如果您的 PR 包含 UI 更改,請使用此清單:

  • 變數名稱實現語意化命名?
  • 測試通過 AA 顏色對比?

@claude
Copy link

claude bot commented Dec 4, 2025

Claude finished @lowrt's task —— View job


Code Review Complete

I've reviewed the PR changes. Here's my analysis:

Tasks

  • Read repository guidelines (CLAUDE.md if exists)
  • Examine changed files
  • Analyze URL changes
  • Provide code review feedback

Summary

This PR removes trailing slashes from the traceBaseUrl getter in two earthquake report model files. The change is minimal, clean, and addresses a URL formatting issue.

Detailed Review

Changes Made

  • lib/api/model/report/earthquake_report.dart:115
  • lib/api/model/report/partial_earthquake_report.dart:116

Both files had the same change:

// Before
return 'https://scweb.cwa.gov.tw/webdata/drawTrace/plotContour/$year/';

// After  
return 'https://scweb.cwa.gov.tw/webdata/drawTrace/plotContour/$year';

Code Quality: ✅ Excellent

Positive aspects:

  1. Consistent fix across both files - The same issue was fixed in both EarthquakeReport and PartialEarthquakeReport classes, maintaining consistency
  2. Clean implementation - The code follows Dart/Flutter best practices with proper getter syntax
  3. No code duplication concerns - While there's duplicate logic between the two classes, extracting traceBaseUrl might be premature optimization given the simple nature of the getter
  4. Proper usage pattern - The URL is correctly used in lines 124, 132, 140 (earthquake_report.dart) and 125, 133, 141 (partial_earthquake_report.dart) where it's concatenated with /$intensityMapImageName, /$pgaMapImageName, and /$pgvMapImageName

Bug Analysis: ✅ No Issues Found

The change fixes a potential bug:

  • Before: URLs would have double slashes: .../$year//$imageName
  • After: URLs are correctly formatted: .../$year/$imageName

This is correct because the image URL getters append /$imageName, so the base URL should not have a trailing slash.

Performance: ✅ No Concerns

  • Getter is lightweight and computed on-demand
  • String interpolation is efficient
  • No performance implications from this change

Security: ✅ No Concerns

  • URLs are constructed from internal data (time.year)
  • No user input injection risks
  • HTTPS protocol is correctly used
  • Domain is properly hardcoded

Test Coverage: ⚠️ Suggestion

Observation: I don't see test file changes in this PR.

Recommendation: Consider adding or updating tests for:

  1. traceBaseUrl getter to verify no trailing slash
  2. intensityMapImageUrl, pgaMapImageUrl, pgvMapImageUrl to ensure final URLs are correctly formatted without double slashes

Example test structure:

test('traceBaseUrl should not have trailing slash', () {
  // Test implementation
  expect(report.traceBaseUrl, endsWith('/2025'));
  expect(report.traceBaseUrl, isNot(endsWith('/2025/')));
});

test('map image URLs should be correctly formatted', () {
  expect(report.intensityMapImageUrl, contains('/2025/'));
  expect(report.intensityMapImageUrl, isNot(contains('/2025//')));
});

Best Practices Alignment: ✅ Good

The code follows Flutter/Dart conventions:

  • Proper use of getters for computed properties
  • Clear naming conventions
  • Null-safety patterns correctly implemented
  • JSON serialization properly configured

Verdict: ✅ Approved with Minor Suggestion

The code is ready to merge. The fix is correct and addresses the URL formatting issue. My only suggestion is to add tests to prevent regression, but this is not a blocker for merging this bug fix.


@lowrt
Copy link
Contributor Author

lowrt commented Dec 4, 2025

@claude tidy up PR title

@lowrt lowrt enabled auto-merge (squash) December 4, 2025 11:58
@claude
Copy link

claude bot commented Dec 4, 2025

Claude finished @lowrt's task —— View job


Tidying Up PR Title ✓

Tasks

  • Check repository guidelines for PR title conventions
  • Review the code changes
  • Analyze current title format
  • Provide recommendation

Analysis

I've analyzed your repository's commit history and found that it follows the conventional commits format. Recent commits show this pattern:

  • fix(model/report): traceBaseUrl
  • fix(assets): location
  • docs: changelog
  • build: 3.1.310

Current PR Title

Current: Fix traceBaseUrl

Recommended PR Title

Recommended: fix(model/report): remove trailing slash from traceBaseUrl

Reasoning

  1. Follows conventional commits: Uses lowercase fix prefix
  2. Includes scope: (model/report) indicates the affected area
  3. Descriptive: Clearly states what was fixed (removing trailing slash)
  4. Consistent: Matches your repository's commit message patterns

How to Update

You can update the PR title by:

  1. Clicking "Edit" next to the PR title at the top of this page, or
  2. Running: gh pr edit 479 --title "fix(model/report): remove trailing slash from traceBaseUrl"

Unfortunately, I don't have permission to directly edit PR titles via the GitHub CLI, but I've provided you with the exact title that would follow your repository's conventions.


@lowrt lowrt changed the title Fix traceBaseUrl Remove trailing slash from traceBaseUrl Dec 4, 2025
@lowrt lowrt merged commit 2db12cf into main Dec 4, 2025
6 checks passed
@lowrt lowrt deleted the fix/url branch December 4, 2025 12:14
@lowrt lowrt added this to the v3.1.4 milestone Dec 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants