Skip to content

Conversation

7dev7urandom
Copy link
Contributor

@7dev7urandom 7dev7urandom commented Sep 30, 2025

Upgrade and test all npm packages as well as docker image. Remove legacy express dependency.
All should be working as far as I can tell, some minor issues may surface as we use it.

Closes #1190, #1191, #1197, #1199, #1200, #1201, #1203, #1204, #1216, #1229, #1231, #1271, #1273

Summary by CodeRabbit

  • Chores

    • Upgraded OpenTelemetry Collector image to 0.136.0.
    • Updated build and framework dependencies (Svelte/SvelteKit, Vite, plugins, Hono, etc.), removed legacy Express packages, and added dependency overrides for more reliable builds.
  • Bug Fixes / UI

    • Organization dropdown now accepts a structured selectProperties prop (name/onchange moved inside it) — behavior and user experience unchanged.

Copy link
Contributor

coderabbitai bot commented Sep 30, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Bumps OpenTelemetry collector image tags to v0.136.0, updates multiple npm devDependencies and overrides, changes OrganizationDropdown props to accept selectProperties?: HTMLSelectAttributes, and updates call sites to pass select attributes/events via selectProperties.

Changes

Cohort / File(s) Summary of edits
OpenTelemetry image bump
Dockerfile.otel, deployment/development/docker-compose.yml
Updated base image / service image tag from otel/opentelemetry-collector-contrib:0.131.x to otel/opentelemetry-collector-contrib:0.136.0. No other Docker/CMD/service configuration changes.
NPM dependency maintenance
package.json
Upgraded multiple devDependencies (Svelte/Vite toolchain, hono, commander, svelvet, etc.), removed express and related types/packages, added devalue, and adjusted overrides (added cookie, repeated nodemailer).
OrganizationDropdown component API
src/lib/components/OrganizationDropdown.svelte
Replaced permissive index-signature props with explicit selectProperties?: HTMLSelectAttributes; destructuring changed to selectProperties = {}; spread on <select> changed from {...rest} to {...selectProperties}.
Dropdown call sites / prop migrations
src/routes/(authenticated)/organizations/[id=idNumber]/settings/+layout.svelte, src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.svelte, src/routes/(authenticated)/users/invite/+page.svelte
Updated OrganizationDropdown usages to pass attributes (e.g., onchange, name) via selectProperties={{ ... }} instead of direct props like onchange or name. Behavior unchanged aside from prop shape adaptation.

Sequence Diagram(s)

sequenceDiagram
  participant Page as Page / Route
  participant OrgDD as OrganizationDropdown
  participant Router as SvelteKit goto

  rect #EBF5FF
    Note right of Page: New prop shape — pass select attributes/events via selectProperties
  end

  Page->>OrgDD: render(selectProperties = { onchange, name, ... })
  OrgDD->>OrgDD: user changes selection (native select)
  OrgDD->>Page: call selectProperties.onchange(event)
  Page->>Router: goto(localizeUrl(...)) 
  Router-->>Page: navigation complete
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • chrisvire

Poem

I twitch my whiskers at a tag bump bright,
From 0.131 to 0.136 in the night.
I tuck props neat into a selectProperties nest,
Hop, refactor, bind — prepare for the test. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning Alongside the required Docker tag updates, this PR includes extensive unrelated changes to package.json and several Svelte components and routes, which fall outside the scope of the linked Docker image upgrade issue. Separate the broad npm and UI changes into a distinct PR or add additional linked issues to cover those modifications, keeping each PR focused on a single coherent objective.
Title Check ❓ Inconclusive The title “Upgrade packages” is overly generic and only references dependency updates, omitting the equally important Docker image bump and legacy express removal described in the PR, so it does not clearly convey the primary scope of the changes. Revise the title to succinctly reflect both the npm dependency upgrades and the Docker image version bump (and removal of express), for example “Bump OpenTelemetry Docker image to v0.136.0 and upgrade npm dependencies.”
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues Check ✅ Passed The only linked issue (#1190) requires updating the otel/opentelemetry-collector-contrib image to v0.136.0; the PR changes the FROM line in Dockerfile.otel and the image tag in docker-compose.yml accordingly, satisfying the linked issue’s objective.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2dc5950 and 1f77e96.

📒 Files selected for processing (2)
  • src/lib/components/OrganizationDropdown.svelte (2 hunks)
  • src/routes/(authenticated)/organizations/[id=idNumber]/settings/+layout.svelte (1 hunks)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/lib/components/OrganizationDropdown.svelte (1)

10-10: Good refactor to explicit selectProperties API.

The change from permissive rest props to an explicit selectProperties object improves API clarity and makes it obvious that these properties are forwarded to the underlying <select> element. The Record<string, unknown> type and default empty object are appropriate.

Consider tightening the type to common HTML select attributes for better type safety:

selectProperties?: React.SelectHTMLAttributes<HTMLSelectElement> | Record<string, unknown>;

However, since this is Svelte (not React), you might want to define a Svelte-specific type or use svelte/elements types if available, or keep the current flexible approach for custom attributes.

Also applies to: 18-18, 22-22

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b24d4eb and 2dc5950.

📒 Files selected for processing (4)
  • src/lib/components/OrganizationDropdown.svelte (1 hunks)
  • src/routes/(authenticated)/organizations/[id=idNumber]/settings/+layout.svelte (1 hunks)
  • src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.svelte (1 hunks)
  • src/routes/(authenticated)/users/invite/+page.svelte (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-22T14:59:53.366Z
Learnt from: FyreByrd
PR: sillsdev/appbuilder-portal#1297
File: src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts:214-221
Timestamp: 2025-09-22T14:59:53.366Z
Learning: In the AppBuilder portal, bulk project actions in the route `/projects/[filter=projectSelector]/[orgId=idNumber]/+page.server.ts` are organization-scoped by design. The route parameter `[orgId=idNumber]` ensures all operations happen within a single organization context, so using the route-level orgId for all projects in bulk actions like `doProjectAction` is correct and safe.

Applied to files:

  • src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build-and-test
🔇 Additional comments (2)
src/routes/(authenticated)/projects/[filter=projectSelector]/[orgId=idNumber]/+page.svelte (1)

178-180: LGTM! API migration to selectProperties.

The OrganizationDropdown usage has been correctly updated to use the new selectProperties prop. The onchange handler (lowercase) is appropriate for the native <select> element.

src/routes/(authenticated)/users/invite/+page.svelte (1)

70-70: LGTM! Correctly migrated to selectProperties.

The name attribute has been correctly moved into the selectProperties object, consistent with the new OrganizationDropdown API.

@7dev7urandom 7dev7urandom merged commit 6910ce4 into develop Sep 30, 2025
4 of 5 checks passed
@7dev7urandom 7dev7urandom deleted the upgrade-packages branch September 30, 2025 22:25
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