Skip to content

feat: align Activity page with Routes design (INC-47) - #60

Open
joaopavila wants to merge 2 commits into
mainfrom
feat/INC-47-activity-page-update
Open

joaopavila wants to merge 2 commits into
mainfrom
feat/INC-47-activity-page-update

Conversation

@joaopavila

@joaopavila joaopavila commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

The Activities page now uses the Routes page’s design language while retaining a list-only layout. Each activity card shows the service summary title, date and time, duration, power profile, distance, and total ascent; upload-status pills use the existing summary.uploadStatus data.

Preserves activity details and deletion, adds keyboard activation, and reuses the shared fold-window hook for scrolling and lazy rendering. Filters, view toggles, and service changes remain outside this PR.

Screenshot

Redesigned Activities page

Storybook preview using sample activity data, captured at 1440 × 900.

Validation

  • Full unit suite passed with CI=true npm test: 571 tests across 64 files.
  • Updated the RouteListPage test mock to include isFreeRideSupported, fixing the pre-existing Free Ride test failure.
  • npm run build passed.
  • Desktop, tablet, and mobile layouts, plus loading and empty states, visually verified.
  • Screenshot captured from an isolated copy containing only the Activity changes.

@joaopavila
joaopavila requested a review from gdoumen as a code owner October 24, 2026 16:22
@joaopavila joaopavila self-assigned this Oct 24, 2026
@gdoumen

gdoumen commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

My Review

This looks really nice. I love it !!

Two things I observed:

  • There is one potential crash (which was actually crashing the Storybook test). I commented this directly in the file.
  • The row height is much bigger than in the RoutesList. And I have the feeling that most of that additional height is just used for empty space. I would either reduce the height to be in line with the RouteList or make better use of the space ( e.g. increase the height of the "Power profile" graph)

AI Review

Verdict: nearly ready. The PR covers everything in the ticket and closely follows the Routes list pattern. Before merge I'd ask for two fixes: restore interaction logging, and split the card into smart and view halves. CI passes on Node 24 and 25, and the branch merges cleanly into main (it's 2 commits behind: a version bump and a services bump).

Against the ticket

Requirement Status
Same design language as Routes ✅ Card grid, breakpoints (1100/650px), Details button, trash button and skeleton mirror RouteTileSummaryView in list layout
List view only, no filters ✅ The unused filter props were removed from ActivitiesScreen
Title from the summary ✅ Safe to drop the old UI fallback to the route title. In services, getPastActivities() strips the Incyclist Ride: prefix, and the summary builder plus the v3 migration already swap the default title for the route title
Date and time, duration, power profile, distance, ascent ✅ All six are shown, with a fallback for missing values and for activities without power
Upload pills only if the service already provides them ✅ summary.uploadStatus is a real Array<UploadInfo> ('success' | 'failure') in services, and names come from apps.getName()
No service changes ✅

The scrolling rewrite now uses the shared useFoldWindow, like RoutesTable. It removes about 130 lines of hand-written fold and swipe code, and it keeps the saved scroll position (getListTop/setListTop).

Should fix

  1. User actions are no longer logged. The codebase guide requires every user interaction to be logged. The old DeleteIcon went through the Icon atom, which logs clicks; the new trash button is a plain <button> and logs nothing. ActivitiesPage.onDeleteHandler doesn't log either, so deleting an activity is now completely unlogged. Clicking the card or pressing Enter/Space isn't logged in the component either; selection is still covered by the page's item selected event. The Routes card it copies logs all three explicitly (tile clicked, key pressed, button clicked/delete in RouteTileSummary/component.jsx). The PR should do the same.

Worth fixing

  1. No <ErrorBoundary>. The Routes view wraps each card in <ErrorBoundary hideOnError>. Here, one malformed activity record can take down the whole list.
  2. The power profile can show "Loading…" forever. getActivityDetails() in services never sends a result in two cases: getWithDetails resolves to nothing, or getActivity(id) isn't found and the error is swallowed. The card then sits on "Loading power profile…" indefinitely. Part of this is a services gap, but a timeout or fallback in the UI would be cheap.
  3. A screenshot is committed to the repo (.github/screenshots/activity-page.png). It's a binary that only exists for the PR description, and nothing else lives under .github/screenshots. Better to attach it to the PR body directly.

Nits

  • The ascent fallback details?.totalElevation is raw metres and always labelled m, so imperial users see metres when the summary has no elevation. The old code had the same problem.
  • The fix to the routeList test mock (isFreeRideSupported) has nothing to do with this ticket. It's small and mentioned in the PR description, so it's fine, but it could have been its own commit or PR.
  • Tests are good: colocated, services mocked, and they cover folding, loading, errors and imperial or zero values. Repo test instruction 13 asks for clearAllMocks in afterEach; these tests do it in beforeEach. Trivial.

const { summary, details: suppliedDetails } = activityInfo ?? {}
const id = summary?.id
const [loaded, setLoaded] = useState(null)
const details = loaded?.id === id ? { ...suppliedDetails, ...loaded.details } : suppliedDetails

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This crashes if loaded is null (default)
replace with loaded?.details

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.

2 participants