fix: return 404 for apps without a setup page#27762
fix: return 404 for apps without a setup page#27762AhmadYasser1 wants to merge 1 commit intocalcom:mainfrom
Conversation
github-actions
Bot
added
Low priority
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
romitg2
left a comment
romitg2
left a comment
There was a problem hiding this comment.
@AhmadYasser1 Thanks for PR! could you please add add before/after visual demo
|
Before 
After 
|
@romitg2 Does this work? |
|
@AhmadYasser1 there seems to be some failing tests related to changes in this PR, can you please fix them. https://github.com/calcom/cal.com/actions/runs/21812508579/job/62929175710?pr=27762 |
dhairyashiil
left a comment
dhairyashiil
left a comment
There was a problem hiding this comment.
The failing test is a known issue, it was also failing on the other PR (linked to issue). Additionally users cannot access the /setup page from the UI for apps without a setup page, so this issue isnt very meaningful to work on.
That said, if you can make the tests pass, I am happy to merge this PR. Otherwise we can close the issue. Let me know you think
|
@romitg2 @dhairyashiil The failing test is The Stripe setup tests all pass locally (15/15). Happy to keep this open if you'd like to merge, or close it if you feel the issue isn't meaningful enough. Let me know! |
Change the app setup getServerSideProps to return `notFound: true` instead of empty props when the app slug has no setup page entry. This shows a proper 404 page instead of a blank screen. Fixes calcom#24128
AhmadYasser1
force-pushed
the
fix/app-setup-404-not-found
branch
from
ab1acd2 to
c060240
Compare
|
@dhairyashiil @romitg2 rebased on latest main to retrigger CI. the failing test (no-show-updated-action integration test) passes on other recently merged PRs like #27814 and #27782, so it looks like it was a flaky run rather than something my change caused. this PR only touches one line in _getServerSideProps.tsx. let me know how it goes! |
|
@dhairyashiil hey, i rebased on latest main to pick up any fixes but the CI is skipping now because the run-ci label was added before the new SHA. could you remove and re-add the run-ci label so CI triggers? thanks! |
dhairyashiil
added
the
run-ci
Graphite App
|
I have tried this multiple times. I told you this is a known issue; tests were failing for this code change: 
I am also aware that it is not directly related to your PR, but I have seen the same pattern in another PR as well, it had the same code change. In my opinion, its not worth the effort to work on this right now. There are many important, higher-priority issues we can focus on. Closing both the PR and the issue. |
3 participants
Summary
/apps/{slug}/setupfor apps that don't have a setup pageChanges
packages/app-store/_pages/setup/_getServerSideProps.tsx: Return{ notFound: true }instead of{ props: {} }when the slug is not inAppSetupPageMapRoot Cause
When visiting
/apps/umami/setup(or any app not inAppSetupPageMap),getServerSidePropsreturns{ props: {} }. TheDynamicComponentthen receives a slug with no matching component and returnsnull, rendering a blank screen. Users report this as a bug (see #23801).Fix
Return
{ notFound: true }for apps not inAppSetupPageMap, which tells Next.js to render the 404 page.Test plan
/apps/umami/setup→ shows 404 page instead of blank screen/apps/stripe/setup→ still works correctly (app is in AppSetupPageMap)Fixes #24128