Skip to content

Add JSON response support for notifications/tray#2398

Merged
alpkeser merged 15 commits intomainfrom
notifications-tray-api
Feb 18, 2026
Merged

Add JSON response support for notifications/tray#2398
alpkeser merged 15 commits intomainfrom
notifications-tray-api

Conversation

@svara
Copy link
Contributor

@svara svara commented Jan 19, 2026

Updated notification/tray JSON for mobile app requirements:

  • Tray API now includes read notifications when include_unread=true
  • ETag varies by include_unread to avoid cache collisions
  • Added controller test for include_unread JSON

json.creator notification.creator, partial: "users/user", as: :user
json.creator do
json.partial! "users/user", user: notification.creator
json.avatar_url user_avatar_url(notification.creator)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jorgemanrubia Do you see any reason why we couldn't include the avatar url in the existing user partial?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@svara should be fine to add it to the user partial

@alpkeser alpkeser force-pushed the notifications-tray-api branch 2 times, most recently from e6e395f to a3aefba Compare January 29, 2026 07:12
@alpkeser alpkeser marked this pull request as ready for review January 29, 2026 07:13
@alpkeser alpkeser force-pushed the notifications-tray-api branch from f604f0e to d954ec9 Compare January 29, 2026 07:26
def show
@notifications = Current.user.notifications.preloaded.unread.ordered.limit(100)
@notifications = unread_notifications
if include_unread?
Copy link
Contributor

@alpkeser alpkeser Jan 29, 2026

Choose a reason for hiding this comment

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

@svara added this query parameter instead of checking for format.json. Let me know if you have any concerns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alpkeser I'm wondering if we'll ever need it, given the fact that we want unreads by default. cc @jayohms

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused. Don't both the web and mobile apps always want unread items here? And just the mobile apps also want read items for the json response? If we used a query param, wouldn't I expect this to be include_read?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doh, I meant read.

If we used a query param, wouldn't I expect this to be include_read?

Yes.

Copy link
Contributor

@alpkeser alpkeser Feb 2, 2026

Choose a reason for hiding this comment

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

Sorry for confusing both of you, my bad. The param should named as include_read.

@alpkeser
Copy link
Contributor

alpkeser commented Feb 3, 2026

Hey @kevinmcconnell, I think we're done with the changes. Could you review this when you have a moment?

@alpkeser
Copy link
Contributor

alpkeser commented Feb 5, 2026

@jhutarek FYI, just deployed this branch to beta2.

@alpkeser alpkeser force-pushed the notifications-tray-api branch from f99d54e to df5057c Compare February 10, 2026 11:39
@alpkeser alpkeser changed the base branch from main to server-side-notification-grouping February 10, 2026 11:40
@alpkeser
Copy link
Contributor

@monorkin and @kevinmcconnell I’ve rebased this branch on top of server-side-notification-grouping. I think we still need this endpoint for the apps, unless we want to incorporate these changes into the notifications endpoint.

@alpkeser
Copy link
Contributor

I'm having trouble to deploy this branch to beta2. I'd appreciate some help 🙂
Enabling saas mode:
bin/rails saas:enable
Deploy:
bin/kamal deploy -d beta2
Output:

...
 INFO [d1c2a21e] Running docker run --detach --restart unless-stopped --name fizzy-web-beta2-df5057c84d1936fbb3d2e5708c685ed02f95bb1f --network kamal --hostname fizzy-beta-app-102.df-iad-int.37signals.com-372eeb641a5b --env KAMAL_CONTAINER_NAME="fizzy-web-beta2-df5057c84d1936fbb3d2e5708c685ed02f95bb1f" --env KAMAL_VERSION="df5057c84d1936fbb3d2e5708c685ed02f95bb1f" --env KAMAL_HOST="fizzy-beta-app-102.df-iad-int.37signals.com" --env KAMAL_DESTINATION="beta2" --env APP_FQDN="beta2.fizzy-beta.com" --env CACHE_NAMESPACE="2" --env RAILS_ENV="beta" --env RAILS_LOG_LEVEL="fatal" --env MYSQL_DATABASE_HOST="fizzy-mysql-primary" --env MYSQL_DATABASE_REPLICA_HOST="fizzy-mysql-replica" --env MYSQL_SOLID_CABLE_HOST="fizzy-mysql-primary" --env MYSQL_SOLID_QUEUE_HOST="fizzy-beta-solidqueue-db-102.df-iad-int.37signals.com" --env MYSQL_SOLID_CACHE_HOST="fizzy-beta-solidcache-db-101.df-iad-int.37signals.com" --env PRIMARY_DATACENTER="true" --env-file .kamal/apps/fizzy-beta2/env/roles/web.env --log-opt max-size="10m" --volume fizzy:/rails/storage --volume $PWD/.kamal/apps/fizzy-beta2/assets/volumes/web-df5057c84d1936fbb3d2e5708c685ed02f95bb1f:/rails/public/assets --label service="fizzy" --label role="web" --label destination="beta2" --label otel_scrape_enabled="true" registry.37signals.com/basecamp/fizzy:df5057c84d1936fbb3d2e5708c685ed02f95bb1f on fizzy-beta-app-102.df-iad-int.37signals.com
  INFO [d1c2a21e] Finished in 0.827 seconds with exit status 0 (successful).
  INFO [378fd265] Running docker container ls --all --filter name=^fizzy-web-beta2-df5057c84d1936fbb3d2e5708c685ed02f95bb1f$ --quiet on fizzy-beta-app-102.df-iad-int.37signals.com
  INFO [378fd265] Finished in 0.616 seconds with exit status 0 (successful).
  INFO [c20589d2] Running docker exec kamal-proxy kamal-proxy deploy fizzy-web-beta2 --target="ab4eb00f2ad8:80" --deploy-timeout="30s" --drain-timeout="30s" --buffer-requests --buffer-responses --log-request-header="Cache-Control" --log-request-header="Last-Modified" --log-request-header="User-Agent" on fizzy-beta-app-102.df-iad-int.37signals.com
 ERROR Failed to boot web on fizzy-beta-app-102.df-iad-int.37signals.com
  INFO First web container is unhealthy on fizzy-beta-app-102.df-iad-int.37signals.com, not booting any other roles
  INFO [60e6e9e1] Running docker container ls --all --filter name=^fizzy-web-beta2-df5057c84d1936fbb3d2e5708c685ed02f95bb1f$ --quiet | xargs docker logs --timestamps 2>&1 on fizzy-beta-app-102.df-iad-int.37signals.com
  INFO First web container is unhealthy, not booting jobs on fizzy-beta-jobs-102.df-iad-int.37signals.com
  INFO [60e6e9e1] Finished in 0.544 seconds with exit status 0 (successful).
 ERROR 
  INFO [3b5a6f84] Running docker container ls --all --filter name=^fizzy-web-beta2-df5057c84d1936fbb3d2e5708c685ed02f95bb1f$ --quiet | xargs docker inspect --format '{{json .State.Health}}' on fizzy-beta-app-102.df-iad-int.37signals.com
  INFO [3b5a6f84] Finished in 0.767 seconds with exit status 0 (successful).
 ERROR null
  INFO [2c86465a] Running docker container ls --all --filter name=^fizzy-web-beta2-df5057c84d1936fbb3d2e5708c685ed02f95bb1f$ --quiet | xargs docker stop on fizzy-beta-app-102.df-iad-int.37signals.com
  INFO [2c86465a] Finished in 10.907 seconds with exit status 0 (successful).
Releasing the deploy lock...
  Finished all in 127.6 seconds
  ERROR (SSHKit::Command::Failed): Exceptions on 2 hosts:
Exception while executing on host fizzy-beta-app-102.df-iad-int.37signals.com: docker exit status: 1
docker stdout: Nothing written
docker stderr: Error: target failed to become healthy within configured timeout (30s)

Exception while executing on host fizzy-beta-jobs-102.df-iad-int.37signals.com: Halted at barrier

@monorkin
Copy link
Contributor

@alpkeser if you rebased this to my branch you should deploy to staging. Deploying to a beta could cause problems in production.

@alpkeser alpkeser force-pushed the notifications-tray-api branch from df5057c to 21acb11 Compare February 11, 2026 15:53
@alpkeser
Copy link
Contributor

@monorkin and @kevinmcconnell I’ve rebased this branch on top of server-side-notification-grouping. I think we still need this endpoint for the apps, unless we want to incorporate these changes into the notifications endpoint.

Hey both, as we’re approaching to the launch, this PR along side with server-side-notification-grouping seems to be one of the last dependencies we need to get in. We’re aiming to have an internal release next Monday. I wanted to check if it works on your end timing-wise.

@monorkin monorkin force-pushed the server-side-notification-grouping branch 3 times, most recently from c799228 to 36ee253 Compare February 12, 2026 09:30
Base automatically changed from server-side-notification-grouping to main February 12, 2026 09:33
@monorkin
Copy link
Contributor

monorkin commented Feb 12, 2026

@alpkeser I squashed the commits in the base branch, you might have to rebase and cherry-pick your commits to get a clean history (Claude is really grate at that if you explain to it what happened).

The #2512 is now live in production, so after you rebase, you can use the betas again for testing.

@alpkeser
Copy link
Contributor

@alpkeser I squashed the commits in the base branch, you might have to rebase and cherry-pick your commits to get a clean history (Claude is really grate at that if you explain to it what happened).

The #2512 is now live in production, so after you rebase, you can use the betas again for testing.

Thank you for the heads up @monorkin 🙏 I'm on it.

svara and others added 7 commits February 12, 2026 14:55
Add avatar url, board name, and column.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Includes assignee users in the card object for display in notification UI.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
These fields were added but are not used by the iOS client.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@alpkeser alpkeser force-pushed the notifications-tray-api branch from 21acb11 to b755522 Compare February 12, 2026 11:56
Copy link
Contributor

@monorkin monorkin left a comment

Choose a reason for hiding this comment

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

@alpkeser great work on this!

I left a few suggestions on how we could improve render times in a few cases, and I had a question about the column field in notification.card.

@alpkeser
Copy link
Contributor

@alpkeser great work on this!

I left a few suggestions on how we could improve render times in a few cases, and I had a question about the column field in notification.card.

Thank you for the review @monorkin 🙏

@alpkeser alpkeser requested a review from monorkin February 16, 2026 15:02
@alpkeser
Copy link
Contributor

Hey @monorkin, I wanted to double-check if there’s anything that needs a follow-up or if this one is good to go.

@monorkin
Copy link
Contributor

@alpkeser we are good to go from my side

@alpkeser alpkeser merged commit 610cf7a into main Feb 18, 2026
12 checks passed
@alpkeser alpkeser deleted the notifications-tray-api branch February 18, 2026 10:20
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.

6 participants

Comments