Skip to content

feat(apps): add installation detail view#74

Merged
vitramir merged 4 commits into
mainfrom
noa/issue-72
Apr 22, 2026
Merged

feat(apps): add installation detail view#74
vitramir merged 4 commits into
mainfrom
noa/issue-72

Conversation

@casey-brooks

Copy link
Copy Markdown
Contributor

Summary

  • add installation detail page with status markdown and audit log
  • link installed apps list to detail page and move status/audit log out of the configure modal
  • update mock server and tests for the new detail view

Testing

  • npm run generate
  • npm run typecheck
  • npm run lint
  • npm run test
  • npm run build
  • npm run test:e2e

#72

@casey-brooks

Copy link
Copy Markdown
Contributor Author

Summary

  • add installation detail page with status markdown and audit log
  • link installed apps list to detail page and move status/audit log out of the configure modal
  • update mock server and tests for the new detail view

Test & Lint Summary

  • npm run generate
  • npm run typecheck
  • npm run lint (no errors)
  • npm run test (14 files, 59 tests passed)
  • npm run build
  • npm run test:e2e (31 passed)

@noa-lucent noa-lucent left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Summary: Installation detail page + moving status/audit log out of the Configure modal looks aligned with #72, and the new unit tests cover the main happy path.

Requesting changes for a couple of correctness issues:

  • React Query cache keys/invalidation: the detail query key doesn’t line up with the existing invalidateQueries(['installations', organizationId]) pattern, so after Configure/Uninstall the detail page can remain stale.
  • Not-found behavior: 404/NotFound from getInstallation() should render the Not Found surface per spec, not the generic error.

See inline comments for concrete suggestions.

Comment thread src/pages/InstallationDetailPage.tsx Outdated
Comment thread src/pages/InstallationDetailPage.tsx
Comment thread src/pages/InstallationDetailPage.tsx
Comment thread src/components/UpdateInstallationDialog.tsx
Comment thread test/e2e/mock-server.mjs Outdated
@casey-brooks

Copy link
Copy Markdown
Contributor Author

Summary

  • align installation detail query key with org-scoped invalidations
  • treat ConnectError NotFound as installation-not-found and avoid generic error
  • make mock server GetInstallation strict on id field

Test & Lint Summary

  • npm run generate
  • npm run typecheck
  • npm run lint (no errors)
  • npm run test (14 files, 60 tests passed)
  • npm run build
  • npm run test:e2e (31 passed)

noa-lucent
noa-lucent previously approved these changes Apr 22, 2026

@noa-lucent noa-lucent left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-review: fixes look good.

  • Detail query key is now org-scoped, so existing invalidateQueries(['installations', organizationId]) refreshes detail/list/overview consistently.
  • ConnectError(Code.NotFound) now renders the Not Found surface (and avoids showing the generic error at the same time).
  • Mock server GetInstallation request shape is strict.

Approving.

@casey-brooks

Copy link
Copy Markdown
Contributor Author

Summary

  • removed the deprecated e2e mock server and added AppsGateway helpers for creating/installing apps
  • added a Playwright spec to snapshot the installation detail page via Argos

Testing

@casey-brooks

Copy link
Copy Markdown
Contributor Author

Summary

  • add explicit app permissions in apps e2e spec to satisfy CreateApp constraints

Testing

@casey-brooks

Copy link
Copy Markdown
Contributor Author

CI status

@vitramir
vitramir merged commit dbb7b44 into main Apr 22, 2026
3 of 5 checks passed
casey-brooks added a commit that referenced this pull request Apr 22, 2026
* feat(apps): add installation detail view

* fix(apps): align installation detail cache

* test(e2e): add installation detail spec

* fix(e2e): add app permissions
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.

3 participants