Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: show available client routes #19050

Merged
merged 11 commits into from Apr 3, 2024
Merged

fix: show available client routes #19050

merged 11 commits into from Apr 3, 2024

Conversation

tltv
Copy link
Member

@tltv tltv commented Mar 27, 2024

Adds client views in the list of available routes when viewing "Route not found" page in development mode. Adds ClientRoutesProvider interface to get available client routes. Client routes means mainly Hilla views, but not limited to.

Fixes: #18857

@@ -79,7 +82,7 @@ public int setRouteNotFoundErrorParameter(BeforeEnterEvent event,
boolean productionMode = event.getUI().getSession().getConfiguration()
.isProductionMode();
String template;
String routes = getRoutes(event);
String routes = getServerRoutes(event);
Copy link
Member

Choose a reason for hiding this comment

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

This should not use only server routes in the end. The logic below requires routes not to be empty if there are server or client views

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, looks like I forgot to rename getServerRoutes back to getRoutes.

Copy link

github-actions bot commented Mar 27, 2024

Test Results

1 099 files  +8  1 099 suites  +8   1h 24m 53s ⏱️ + 1m 6s
6 937 tests +1  6 888 ✅ +1  49 💤 ±0  0 ❌ ±0 
7 292 runs  +8  7 231 ✅ +8  61 💤 ±0  0 ❌ ±0 

Results for commit c2e4c20. ± Comparison against base commit 85f4a25.

♻️ This comment has been updated with latest results.

@vaadin-bot vaadin-bot added +1.0.0 and removed +0.1.0 labels Mar 27, 2024
@tltv tltv force-pushed the fix/18857-list-hilla-routes branch from dfea9eb to dfc1f5d Compare March 27, 2024 14:10
@vaadin-bot vaadin-bot added +0.1.0 and removed +1.0.0 labels Mar 27, 2024
@tltv tltv marked this pull request as ready for review March 27, 2024 14:54
@tltv tltv force-pushed the fix/18857-list-hilla-routes branch from dfc1f5d to e9378c4 Compare March 28, 2024 10:07
@caalador
Copy link
Contributor

caalador commented Apr 2, 2024

Does and should ClientRoutesProvider also handle user defined routes setup from routes.tsx?
Is this feature only for file router setups?

@mshabarov mshabarov requested a review from caalador April 2, 2024 06:44
@tltv
Copy link
Member Author

tltv commented Apr 2, 2024

Does and should ClientRoutesProvider also handle user defined routes setup from routes.tsx? Is this feature only for file router setups?

It's meant for both. So far I've tested it with Hilla by letting ClientRouteRegistry to implement ClientRouteProvider and it returns both.

I noticed that $index.tsx and $layout.tsx are still listed. $layout.tsx should be always hidden as it's not a real path afaik, but $index.tsx path should show up to cover up cases when it's in other places than root.

@tltv
Copy link
Member Author

tltv commented Apr 2, 2024

There was discussion in Flow daily meeting about the Spring security to be taken in account with this, but as ClientRoutesProvider returns a full list of available routes to build a link list for development mode, no access checking is needed in this use case. For other use case that needs security checking, we can add another method in the interface for a list of accessible paths if needed.

Comment on lines 32 to 34
default List<String> getClientRoutes() {
return List.of();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This perhaps shouldn't have a default method and instead require that it is always implemented.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, no need for the default impl anymore. It was there for a reason first.

tltv added 10 commits April 2, 2024 13:58
Adds client views in the list of available routes when viewing "Route not found" page in development mode.
Adds ClientRoutesProvider interface to get available client routes. Client routes means mainly Hilla views, but not limited to.

Fixes: #18857
Can't use @ConditionalOnMissingBean as the bean from Hilla dependencies is not reached from here leading to multiple beans which will break things. Therefore, allowing just one implementation of ClientRoutesProvider when Hilla is in the classpath.
@tltv tltv force-pushed the fix/18857-list-hilla-routes branch from ba837df to 3e31550 Compare April 2, 2024 11:47
Copy link

sonarcloud bot commented Apr 2, 2024

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@tltv tltv merged commit 43447ae into main Apr 3, 2024
26 checks passed
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.4.0.alpha19 and is also targeting the upcoming stable 24.4.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Available routes don't show Hilla views
4 participants