feat: refactor to add workspace for future features#6
Conversation
- Added workspace context management with middleware for setting and switching workspaces. - Introduced `HasWorkspaceScope` trait for automatic workspace filtering on database connections. - Updated database connection handling to associate connections with workspaces instead of users. - Enhanced PostgreSQL connection status handling and UI components for better user experience. - Updated migrations to support new workspace and membership structures. - Refactored related components and API routes to accommodate workspace-based logic.
- Implemented workspace selection UI in the sidebar with a new `TeamSelector` component. - Enhanced user model to include relationships for workspaces and memberships. - Updated middleware to share user workspaces data with the frontend. - Refactored related components to utilize the new workspace management features.
- Added `@radix-ui/react-alert-dialog` for enhanced alert dialog functionality. - Updated various dependencies including `@inertiajs/react`, `laravel-vite-plugin`, and `react-hook-form` to their latest versions for improved performance and features. - Refactored components for better code consistency and readability. - Fixed minor bugs in the Postgres connection handling and user workspace data sharing.
|
Tip For best results, initiate chat on the files or code changes. @adiologydev Hi there! How can I assist you on this PR? Let me know if you’d like a code review, help writing tests, planning refactors, or anything else. |
@coderabbitai Can you review this PR? |
|
Tip For best results, initiate chat on the files or code changes. @adiologydev I’m pulling the latest diff for this PR so I can provide a thorough review. One moment while I gather the changes… |
@coderabbitai any updates on the PR review? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 285 🏁 Script executed: Length of output: 1702 🏁 Script executed: Length of output: 67372 @adiologydev here’s an initial high-level review of the PR. ──────────────────────────────────────────────────────── • ULID overhaul – • Middleware additions – • Dependency bumps – Large JS & composer lockfile upgrades (Laravel 12.18, React 19.1, etc.). I didn’t spot breaking-change usage but recommend running the full test suite. ────────────────────────────────────────────────────────
──────────────────────────────────────────────────────── • Decide which workspace middleware to keep and register only that. Let me know if you’d like me to draft specific code changes, write PHPUnit tests, or review the React side in detail! |
- Added `barryvdh/laravel-debugbar` for improved debugging capabilities. - Updated `laravel/pint` to version 1.22 for enhanced code quality checks. - Refactored workspace context management in middleware and models for better clarity and performance. - Removed unused `SetWorkspaceContext` middleware to streamline the codebase. - Improved error handling in PostgreSQL connection and query execution. - Cleaned up code formatting for consistency across various files.
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces workspace-based multi-tenancy to the application. Models, controllers, and middleware are refactored to associate database connections and queries with workspaces instead of individual users. New models and migrations implement workspaces, roles, and memberships. The frontend adds workspace selection, updates connection handling to use workspace-scoped IDs, and centralizes connection status display. Supporting configuration, middleware, and event listeners are added for workspace management and context switching. Changes
Sequence Diagram(s)Workspace Context and Connection AccesssequenceDiagram
participant User
participant Middleware
participant Controller
participant DatabaseConnection
participant Workspace
User->>Middleware: Sends request (optionally with switch_workspace)
Middleware->>Middleware: Checks authentication and workspace membership
Middleware->>Middleware: Sets current_workspace_id in context
Middleware->>Controller: Passes request with workspace context
Controller->>DatabaseConnection: Queries connections filtered by workspace_id
DatabaseConnection->>Workspace: BelongsTo relationship for workspace
Controller->>User: Checks if user's current_workspace_id matches connection's workspace_id
Controller-->>User: Returns workspace-scoped data
User Registration and Workspace CreationsequenceDiagram
participant User
participant RegisteredEventListener
participant Workspace
participant WorkspaceMembership
User->>RegisteredEventListener: Triggers Registered event
RegisteredEventListener->>Workspace: Creates new workspace (owner = user)
RegisteredEventListener->>WorkspaceMembership: Creates membership (role = owner)
RegisteredEventListener->>User: Sets current_workspace_id
RegisteredEventListener-->>User: Registration completes with workspace context
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🔭 Outside diff range comments (6)
resources/js/stores/postgres-store.ts (2)
14-21: 🛠️ Refactor suggestionFix type definitions for ULID identifiers
QueryHistoryItem.idanddatabase_connection_idare ULIDs (strings), not numbers. Update their types accordingly:-export interface QueryHistoryItem { - id: number; - database_connection_id: number; +export interface QueryHistoryItem { + id: string; + database_connection_id: string;
168-176:⚠️ Potential issueStandardize route parameter naming
Theroute("postgres.query.execute")call usesconnection_id, whereas other route helpers useconnection. Confirm the expected route parameter inroutes/web.phpand make it consistent (e.g., useconnectionfor both).database/migrations/2025_03_13_184947_create_database_connections_table.php (1)
14-22: 🛠️ Refactor suggestionConsider encrypting the
passwordcolumn and adding workspace-scoped uniqueness.
$table->string('password')stores credentials as plain text. Either:-$table->string('password'); +$table->text('password_encrypted');and encrypt/decrypt in the model accessor/mutator, or leverage Laravel’s built-in encrypted cast.
- With multi-tenancy you probably want a uniqueness guard such as
unique(['workspace_id', 'name'])to prevent duplicate connection names inside the same workspace.Both changes can be done in a follow-up migration before release.
app/Http/Controllers/Postgres/PostgresQueryController.php (1)
58-62:⚠️ Potential issueValidation rule should match ULID format.
connection_idcolumn is a ULID (char(26)), yet the rule only assertsstring.
Laravel provides aulidrule:- $request->validate([ - 'connection_id' => 'required|string', + $request->validate([ + 'connection_id' => 'required|ulid',This prevents malformed IDs from hitting the database.
app/Models/DatabaseConnection.php (1)
12-28:⚠️ Potential issueAvoid mass-assignment of
workspace_id; rely on the trait instead
workspace_idis now assignable via$fillable.
Because the model also usesHasWorkspaceScope, an attacker could forge a request that overrides the ID before the trait’screatingcallback runs (or if the trait only sets the ID when it isnull). That breaks tenant isolation.- 'metadata', - 'workspace_id', + 'metadata',Let the trait populate
workspace_idexclusively, or at minimum validate the value against the current workspace in a model event listener.app/Http/Controllers/Postgres/PostgresConnectionController.php (1)
307-308: 💡 Verification agent🧩 Analysis chain
Verify if user ID is still needed for row insertion.
While the authorization uses workspace context, the insertRow service call still passes the user ID. This might be for audit logging purposes.
🏁 Script executed:
#!/bin/bash # Description: Check how the user ID is used in the PostgresService insertRow method # Search for the insertRow method implementation ast-grep --pattern 'function insertRow($_connection, $_database, $_schema, $_table, $_data, $_userId) { $$$ }' # Also check with different parameter names rg -A 10 "function insertRow.*userId" app/Services/Length of output: 168
I wasn’t able to locate the
insertRowimplementation or any usage of the passed user ID. Let’s broaden our search to ensure we catch all definitions and calls:
🏁 Script executed:
#!/bin/bash # Find any definitions of insertRow in the Services directory rg -n "function insertRow" -R app/Services -A 5 # Find all calls to insertRow to see parameter usage rg -n "insertRow\(" -R app -A 3Length of output: 893
Let’s correct the ripgrep flags and search the entire PHP code for both the definition and calls of
insertRow:
🏁 Script executed:
#!/bin/bash # Find the insertRow method definition in Services rg -n "function insertRow" -g "*.php" app/Services -A 5 # Find all calls to insertRow throughout the app to see passed parameters rg -n "insertRow\(" -g "*.php" app/ -A 3Length of output: 1807
Remove the now-unused user ID argument from the insertRow call
The
PostgresService::insertRowsignature accepts only five parameters (connection,database,schema,table,data), so passing the sixth argument ($request->user()->id) will cause an argument-count error. Please remove it:• File:
app/Http/Controllers/Postgres/PostgresConnectionController.php
Around lines 301–308Suggested diff:
- $result = $this->postgresService->insertRow( - $connection, - $request->input('database'), - $request->input('schema'), - $request->input('table'), - $request->input('data'), - $request->user()->id - ); + $result = $this->postgresService->insertRow( + $connection, + $request->input('database'), + $request->input('schema'), + $request->input('table'), + $request->input('data') + );If audit logging is still required, consider handling it inside the service or via a dedicated event/listener instead of passing the user ID here.
♻️ Duplicate comments (5)
resources/js/stores/postgres-store.ts (5)
188-195: Standardize route parameter naming
route("postgres.query.history")usesconnectionas the parameter key, but the execute route usesconnection_id. Align parameter keys across all route helpers.
297-302: Standardize route parameter naming
Verify the route parameter key forpostgres.schemasmatches the backend definition (likely{connection}) and unify naming.
330-334: Standardize route parameter naming
Ensurepostgres.tablesroute parameter naming is consistent with other endpoints.
359-366: Standardize route parameter naming
Ensurepostgres.columnsendpoint uses the correct route parameter key for connection (should match backend route model binding).
368-376: Standardize route parameter naming
Ensurepostgres.dataendpoint parameter aligns with the route definition ({connection}vsconnection_id).
🧹 Nitpick comments (22)
resources/js/components/user-menu-content.tsx (1)
25-28: Label hard-coded; consider i18n key instead
"User Settings"is now hard-coded, whereas the rest of the app appears to rely on translation keys in many places. If you intend to localise the UI, swap the literal for a translation helper (e.g.t("user.settings")).bootstrap/app.php (1)
24-25: Ordering hint
WorkspaceMiddlewareruns afterAddLinkHeadersForPreloadedAssets. If any link headers rely on workspace context (unlikely, but possible when preloading tenant-specific assets) you may want to moveWorkspaceMiddlewareearlier.resources/js/types/postgres.d.ts (1)
55-62: Consider camelCase mapping or consistent naming
The newConnectionStatusinterface uses snake_case to match backend JSON. For cleaner TS usage, consider converting to camelCase properties in the store or use a transformation layer.resources/js/pages/postgres/explorer.tsx (1)
165-170:connectionIdprop may be redundant.
<PostgresContent … connectionId={connection.id} connection={connection} />passes both the id and the full object. Unless the child component explicitly needs only the primitive id for memoisation, one prop is enough.-connectionId={connection.id}inside
PostgresContentsimplifies the API surface.docker/production/Dockerfile (2)
40-43: DuplicateWORKSPACE_ENABLEDdefinition – keep one canonical source.
WORKSPACE_ENABLED=falseis defined in both the build and runtime stages. That’s fine, but if you intend to toggle it at deploy time you’ll need to override two ENV declarations. Consider setting it only in the final stage and passing it duringdocker buildfor compile-time purposes.
100-104: Hard-codingSSL_MODE=off.If SSL may be required in certain deployments expose this as a build-time ARG/ENV instead of a fixed value here to avoid rebuilding images just to flip the switch.
composer.json (1)
55-57: Convenience script naming – match Pint’s convention.Laravel Pint already ships with a default
vendor/bin/pintexecutable; many teams alias it toformat. Consider:-"pint": ["pint"] +"format": ["pint"]Minor, but keeps the scripts section self-documenting.
resources/js/types/index.d.ts (2)
26-31:Workspace.rolecould benefit from an enum.
role: stringhas no type safety. Defining:export type WorkspaceRole = "owner" | "admin" | "member" | "viewer";and using
role: WorkspaceRole;prevents accidental misspellings.
39-40: Consider renamingalltolistfor clarity.
workspaces.allreads oddly in code (workspaces.all.map(...)). A more conventional shape might be{ items: Workspace[] }or{ list: Workspace[] }, but this is purely cosmetic.app/Models/Workspace.php (1)
14-19: Consider exposing factory & slug semantics.
- The model omits the
HasFactorytrait; this makes seeding / model factories less ergonomic.slugis listed as fillable but no mechanism (observer / mutator) guarantees uniqueness or automatic generation.
Leaving this to callers increases the risk of duplicate slugs.use Illuminate\Database\Eloquent\Relations\HasMany; +use Illuminate\Database\Eloquent\Factories\HasFactory; class Workspace extends Model { - use HasUlid; + use HasUlid, HasFactory;A simple
creatingcallback could populate the slug from the name if absent.app/Http/Controllers/Postgres/PostgresQueryController.php (1)
38-41: Comment is outdated — now workspace-level auth, not user-level.The code correctly checks
workspace_id, but the inline comment still says “their own connections”.
Update to avoid confusion for future maintainers.app/Concerns/HasWorkspaceScope.php (1)
13-17: Avoid duplicateContext::getcalls & tighten guard.Calling
Context::gettwice incurs needless work. Prefer caching & an explicithascheck:- static::addGlobalScope('workspace', function (Builder $builder) { - if ($workspaceId = Context::get('workspace_id')) { + static::addGlobalScope('workspace', function (Builder $builder) { + if (Context::has('workspace_id')) { + $workspaceId = Context::get('workspace_id'); $builder->where('workspace_id', $workspaceId); }resources/js/components/app-sidebar.tsx (1)
50-52: Null-safety whenworkspacesprop is absent.
usePage<SharedData>().propsmight not includeworkspacesduring SSR errors or legacy pages.
Destructure defensively:- const { workspaces } = usePage<SharedData>().props; + const { workspaces = { enabled: false, data: [] } } = usePage<SharedData>().props;Prevents runtime crashes in edge cases.
app/Models/User.php (1)
71-74: Duplicate accessor:workspaceMemberships()duplicates (old)workspaces()purpose
If you adopt the change above,workspaceMemberships()becomes the clear place to access membership rows.
Consider renaming tomemberships()for brevity or removing the earlier mis-named method entirely.resources/js/components/workspace-selector.tsx (1)
7-18: Component/product naming drift (TeamSelectorvs. workspaces)
The backend, config and permissions consistently talk about “workspace”. Consider renaming the component toWorkspaceSelectorfor clarity and future grep-ability.app/Http/Middleware/WorkspaceMiddleware.php (1)
24-34: Single extra query per request – eager-load memberships or cache ID list
$user->workspaces()->where('workspace_id', …)->exists()runs an additional query every time the user clicks a workspace link. With the relationship fix (belongsToMany) you can instead rely on an in-memory collection:if ($user->workspaces->contains('id', $workspaceId)) { … }or cache the allowed IDs in session. This removes one DB hit from every navigation.
app/Listeners/RegisteredEvent.php (1)
28-42: Wrap workspace/user bootstrap in a DB transaction
User creation fires theRegisteredevent inside a transaction, but this listener performs three more write queries. If any of them fails, data may be left half-created (e.g. workspace without membership).DB::transaction(function () use ($user) { $workspace = Workspace::create([...]); WorkspaceMembership::create([...]); $user->forceFill(['current_workspace_id' => $workspace->id])->save(); });Import
Illuminate\Support\Facades\DB;at the top.resources/js/pages/postgres/postgres.tsx (1)
7-11: Remove unuseddayjsimport to satisfy linters
dayjsandrelativeTimeare imported and extended but no longer referenced.-import dayjs from "dayjs"; -import relativeTime from "dayjs/plugin/relativeTime"; - -dayjs.extend(relativeTime);This keeps the bundle lean and avoids
biome/eslintwarnings.app/Models/WorkspaceMembership.php (1)
44-57: Replace magic strings with constants / enumHard-coding
'owner' | 'admin' | 'member'in three separate helpers is error-prone. Consider:enum WorkspaceRole: string { case Owner = 'owner'; case Admin = 'admin'; case Member = 'member'; } public function isOwner(): bool { return $this->role === WorkspaceRole::Owner->value; }This guarantees consistency across the codebase.
routes/web.php (1)
20-22: Inconsistent param naming ({id}vs{connection})All other endpoints use
{connection}and leverage implicit model binding.
Consider makingconnectionStatususe{connection}too for consistency and automatic authorization.resources/js/components/ui/alert-dialog.tsx (1)
54-57: Very long className string – consider extracting to a constantThis improves readability and keeps future tweaks in one place.
const contentClasses = "bg-background ... sm:max-w-lg" <AlertDialogPrimitive.Content className={cn(contentClasses, className)} ... />app/Http/Controllers/Postgres/PostgresConnectionController.php (1)
124-134: Update comment to reflect workspace-based ownership.The comment mentions "authenticated user" but the implementation associates connections with workspaces.
-// Ensure the connection is created for the authenticated user only +// Create the connection for the user's current workspace
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
bun.lockis excluded by!**/*.lockcomposer.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
app/Concerns/HasUlid.php(1 hunks)app/Concerns/HasWorkspaceScope.php(1 hunks)app/Http/Controllers/Postgres/PostgresConnectionController.php(8 hunks)app/Http/Controllers/Postgres/PostgresQueryController.php(3 hunks)app/Http/Middleware/HandleInertiaRequests.php(2 hunks)app/Http/Middleware/WorkspaceMiddleware.php(1 hunks)app/Listeners/RegisteredEvent.php(1 hunks)app/Models/DatabaseConnection.php(3 hunks)app/Models/DatabaseQuery.php(1 hunks)app/Models/Role.php(1 hunks)app/Models/User.php(2 hunks)app/Models/Workspace.php(1 hunks)app/Models/WorkspaceMembership.php(1 hunks)bootstrap/app.php(2 hunks)composer.json(2 hunks)config/workspace.php(1 hunks)database/migrations/0001_01_01_000000_create_users_table.php(2 hunks)database/migrations/2025_03_13_184947_create_database_connections_table.php(1 hunks)database/migrations/2025_03_14_185943_create_database_queries_table.php(1 hunks)database/migrations/2025_05_29_133435_create_workspaces_table.php(1 hunks)docker/production/Dockerfile(2 hunks)package.json(2 hunks)resources/js/components/app-sidebar.tsx(3 hunks)resources/js/components/database/sql-error-display.tsx(1 hunks)resources/js/components/postgres/postgres-connection.tsx(1 hunks)resources/js/components/ui/alert-dialog.tsx(1 hunks)resources/js/components/user-menu-content.tsx(1 hunks)resources/js/components/workspace-selector.tsx(1 hunks)resources/js/pages/postgres/explorer.tsx(2 hunks)resources/js/pages/postgres/postgres.tsx(2 hunks)resources/js/stores/postgres-store.ts(6 hunks)resources/js/types/database.d.ts(1 hunks)resources/js/types/index.d.ts(1 hunks)resources/js/types/postgres.d.ts(1 hunks)routes/web.php(1 hunks)storage/debugbar/.gitignore(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
bootstrap/app.php (1)
app/Http/Middleware/WorkspaceMiddleware.php (1)
WorkspaceMiddleware(11-44)
app/Models/Role.php (1)
app/Models/WorkspaceMembership.php (2)
WorkspaceMembership(8-58)getPermissionsAttribute(39-42)
resources/js/components/workspace-selector.tsx (2)
resources/js/types/index.d.ts (1)
SharedData(33-40)app/Models/User.php (2)
workspaces(61-64)currentWorkspace(66-69)
app/Models/WorkspaceMembership.php (3)
app/Models/Workspace.php (1)
Workspace(10-30)app/Models/User.php (1)
User(13-75)app/Models/Role.php (2)
Role(8-27)getPermissionsAttribute(23-26)
app/Http/Middleware/HandleInertiaRequests.php (2)
app/Models/User.php (4)
User(13-75)workspaceMemberships(71-74)workspaces(61-64)currentWorkspace(66-69)app/Models/WorkspaceMembership.php (3)
user(28-31)workspace(23-26)role(33-36)
resources/js/components/postgres/postgres-connection.tsx (3)
app/Models/DatabaseConnection.php (1)
DatabaseConnection(10-53)resources/js/types/database.d.ts (1)
DatabaseConnection(21-29)resources/js/types/postgres.d.ts (1)
ConnectionStatus(56-61)
resources/js/pages/postgres/postgres.tsx (2)
app/Models/DatabaseConnection.php (1)
DatabaseConnection(10-53)resources/js/types/database.d.ts (1)
DatabaseConnection(21-29)
routes/web.php (2)
app/Http/Controllers/Postgres/PostgresConnectionController.php (1)
PostgresConnectionController(13-443)app/Http/Controllers/Postgres/PostgresQueryController.php (1)
PostgresQueryController(14-163)
🔇 Additional comments (15)
storage/debugbar/.gitignore (1)
1-2: Ignored correctly – no further action neededAdding the pattern to keep the directory out of VCS (while retaining the
.gitignorefile itself) is standard practice for Debugbar storage.resources/js/types/database.d.ts (1)
21-23:Details
✅ Verification successful
Verify all frontend code migrated from
ulid→idChanging the primary identifier is breaking if any leftover code still reads
connection.ulid. A quick grep helps:Please confirm only valid historical references remain (e.g. model-level).
🏁 Script executed:
rg '\.ulid\b' --type ts --type tsx --type jsLength of output: 74
🏁 Script executed:
#!/bin/bash # Search for .ulid references in TS, TSX, and JS files using file globs rg '\.ulid\b' -g '*.ts' -g '*.tsx' -g '*.js'Length of output: 46
All
.ulidreferences removed from frontend code
Verified withrg '\.ulid\b'across all.ts,.tsx, and.jsfiles—no occurrences found. Migration toidis complete; no further action required.bootstrap/app.php (1)
5-6:Details
❌ Incorrect review comment
Potential duplicate workspace context middleware
WorkspaceMiddlewareis now imported, butSetWorkspaceContext(added elsewhere in the PR) still exists. Having both in the web stack will execute two similar DB look-ups on every request.Action: pick one (the richer
WorkspaceMiddlewareseems sufficient) and remove / un-register the other to avoid redundant queries.
🏁 Script executed:
#!/bin/bash echo "=== Searching for SetWorkspaceContext definitions and usages ===" rg "SetWorkspaceContext" -n . echo -e "\n=== Searching for WorkspaceMiddleware definitions and usages ===" rg "WorkspaceMiddleware" -n . echo -e "\n=== Inspecting middleware registration in bootstrap/app.php ===" sed -n '1,200p' bootstrap/app.php | grep -n -C3 "middleware"Length of output: 1147
No duplicate workspace middleware detected
I searched the codebase for any
SetWorkspaceContextclass or registration and found zero references—onlyWorkspaceMiddlewareis imported and registered inbootstrap/app.php. There’s no second “workspace” middleware to remove, so no action is required here.Likely an incorrect or invalid review comment.
app/Concerns/HasUlid.php (1)
13-15: ULID now generated intoid– audit migrations & castsGood move toward a single primary key column. Double-check:
- All migrations now use
ulid('id')->primary().- Models have
public $incrementing = false;andprotected $keyType = 'string';as needed.- Old
ulidcolumns are dropped or aliased.A brief migration audit will prevent silent key mismatches.
resources/js/components/database/sql-error-display.tsx (1)
40-46: Consistent high-contrast text styling
The update to usetext-gray-900for light mode paired withdark:text-gray-200ensures good readability across themes. No issues detected.config/workspace.php (1)
3-17: Configuration structure looks solid
The newworkspace.phpconfig properly defines theenabledflag and role permissions. Ensure environment variable documentation is updated accordingly.app/Models/DatabaseQuery.php (1)
5-12: Ensure ULID trait configuration
The addition ofHasUlidaligns with the global identifier strategy. Please verify that the trait sets$incrementing = falseand$keyType = 'string'on the model to properly support ULIDs.resources/js/pages/postgres/explorer.tsx (1)
24-27: Route param update – verify backend route signature.
route("postgres.explore", { connection: connection.id })assumes the Laravel route parameter is now{connection}(no:ulidconstraint). Double-check thatroutes/web.phphas been updated accordingly, otherwise a 404 will occur.composer.json (1)
14-19: Debugbar in dev only – ensure it never leaks to prod.
barryvdh/laravel-debugbaris great for local debugging but can reveal sensitive queries in production if accidentally enabled. Verify thatAPP_DEBUGBAR_ENABLED=false(or the service provider is conditioned onapp()->isLocal()).app/Http/Controllers/Postgres/PostgresQueryController.php (1)
83-91: Potential 255-char truncation when persisting queries.
database_queries.queryis defined asstring(see migration) which maps tovarchar(255).
SQL editors routinely exceed that length, so longer statements will be cut and log integrity lost.
Upgrade the column totextand adjust the migration accordingly.app/Models/User.php (1)
6-16: Verify ULID migration vs. external API expectations
HasUlidis now mixed intoUser, yet external callers will still see/accept the numericidcolumn. Double-check that:
- the users table migration sets
idas ULID (ulid('id')->primary()),- any foreign keys now reference
CHAR(26)strings, and- no code (e.g. policies, route-model binding) still assumes an auto-increment integer.
app/Models/DatabaseConnection.php (1)
49-52: LGTM – correct relationship renameThe new
workspace()relation matches the foreign key and keeps IDE type-hints intact.package.json (1)
41-57: Confirm React 19 ecosystem readiness before merging
react/react-domare bumped to 19.1.x, but React 19 is still in RC/beta for most packages.
Down-stream libraries (@inertiajs/react, Radix, etc.) may still peer-depend on^18. Build or run-time conflicts are likely.Please verify:
pnpm i --strict-peer-deps(or equivalent) installs without warnings.- All E2E/UI tests pass in CI.
- SSR still works if used (React 19 alters
use/ suspense semantics).Roll back to the latest 18.x stable if any incompatibility surfaces.
database/migrations/0001_01_01_000000_create_users_table.php (1)
15-33: ULID migration looks correctPrimary and foreign ULIDs are defined with Laravel helpers; no issues spotted.
resources/js/pages/postgres/postgres.tsx (1)
24-27: Route param change: ensure backend route expectsidnotulid
route("postgres.explore", { connection: connection.id })now sends the numeric/stringid.
Double-check the Laravel route definition (routes/web.php) and controller (PostgresConnectionController) were updated to use{connection:id}orRoute::modelbinding onid.
| public function members(): HasMany | ||
| { | ||
| return $this->hasMany(WorkspaceMembership::class); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Membership relation likely needs BelongsToMany instead of HasMany.
members() returns a HasMany to WorkspaceMembership, so the actual User models are two hops away.
If the intention is Workspace -> users you probably want:
return $this->belongsToMany(User::class, 'workspace_memberships');Leaving it as-is forces every call‐site to traverse the pivot manually and defeats eager-loading convenience.
🤖 Prompt for AI Agents
In app/Models/Workspace.php around lines 26 to 29, the members() method
currently returns a HasMany relation to WorkspaceMembership, but it should
return a BelongsToMany relation directly to User through the
workspace_memberships pivot table. Change the return type to BelongsToMany and
update the relation to use belongsToMany(User::class, 'workspace_memberships')
so that you can directly access users and benefit from eager loading without
manually traversing the pivot.
| $connection = DatabaseConnection::where('id', $request->input('connection_id'))->firstOrFail(); | ||
|
|
||
| // Ensure the user can only access their own connections | ||
| if ($connection->user_id !== $request->user()->id) { | ||
| if ($connection->workspace_id !== $request->user()->current_workspace_id) { | ||
| return response()->json([ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Early exit still allows N + 1 lookup when mismatched.
You fetch the DatabaseConnection and only then assert workspace membership.
Consider folding the workspace filter into the query to avoid an unnecessary DB hit:
-$connection = DatabaseConnection::where('id', $request->input('connection_id'))->firstOrFail();
+$connection = DatabaseConnection::where('id', $request->input('connection_id'))
+ ->where('workspace_id', $request->user()->current_workspace_id)
+ ->firstOrFail();One query, one round-trip, clearer intent.
🤖 Prompt for AI Agents
In app/Http/Controllers/Postgres/PostgresQueryController.php around lines 65 to
69, the code fetches the DatabaseConnection by ID first and then checks if the
workspace_id matches the user's current workspace, causing an unnecessary
database query if the workspace does not match. To fix this, modify the query to
include a condition that filters by both the connection ID and the user's
current workspace ID in a single query, ensuring only connections belonging to
the user's workspace are retrieved and preventing extra database lookups.
| $table->ulid('id')->primary(); | ||
| $table->foreignUlid('database_connection_id')->constrained('database_connections')->cascadeOnDelete(); | ||
| $table->string('query'); | ||
| $table->enum('executor', ['user', 'system']); | ||
| $table->foreignIdFor(User::class, 'executor_id')->nullable()->constrained()->cascadeOnDelete(); | ||
| $table->foreignUlid('executor_id')->nullable()->constrained('users')->cascadeOnDelete(); | ||
| $table->json('parameters')->nullable()->comment('JSON encoded parameters required for the query'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
query column too small & lacks index for fast history lookups.
stringcaps at 255 chars – insufficient for many SQL statements.- History endpoints order by
created_at; adding an index dramatically improves performance.
- $table->string('query');
+ $table->text('query');
+
+ // index for /history endpoints
+ $table->index('created_at');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $table->ulid('id')->primary(); | |
| $table->foreignUlid('database_connection_id')->constrained('database_connections')->cascadeOnDelete(); | |
| $table->string('query'); | |
| $table->enum('executor', ['user', 'system']); | |
| $table->foreignIdFor(User::class, 'executor_id')->nullable()->constrained()->cascadeOnDelete(); | |
| $table->foreignUlid('executor_id')->nullable()->constrained('users')->cascadeOnDelete(); | |
| $table->json('parameters')->nullable()->comment('JSON encoded parameters required for the query'); | |
| $table->ulid('id')->primary(); | |
| $table->foreignUlid('database_connection_id')->constrained('database_connections')->cascadeOnDelete(); | |
| - $table->string('query'); | |
| + $table->text('query'); | |
| + | |
| + // index for /history endpoints | |
| + $table->index('created_at'); | |
| $table->enum('executor', ['user', 'system']); | |
| $table->foreignUlid('executor_id')->nullable()->constrained('users')->cascadeOnDelete(); | |
| $table->json('parameters')->nullable()->comment('JSON encoded parameters required for the query'); |
🤖 Prompt for AI Agents
In database/migrations/2025_03_14_185943_create_database_queries_table.php
around lines 15 to 20, the 'query' column is defined as a string which limits it
to 255 characters, too small for many SQL statements. Change the 'query' column
type to 'text' to allow longer queries. Additionally, add an index on the
'created_at' column to improve performance of history lookups that order by this
field.
| public function workspaces(): HasMany | ||
| { | ||
| return $this->hasMany(WorkspaceMembership::class); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
workspaces() points to memberships – rename or change relation type
The method returns HasMany of WorkspaceMembership, not actual Workspace models.
This is confusing for anyone calling $user->workspaces() expecting real workspaces.
Recommended fix:
-use Illuminate\Database\Eloquent\Relations\HasMany;
+use Illuminate\Database\Eloquent\Relations\BelongsToMany;
...
- public function workspaces(): HasMany
- {
- return $this->hasMany(WorkspaceMembership::class);
- }
+ /**
+ * Workspaces the user belongs to.
+ */
+ public function workspaces(): BelongsToMany
+ {
+ return $this->belongsToMany(
+ Workspace::class,
+ 'workspace_memberships'
+ )->withPivot('role')->withTimestamps();
+ }This delivers the expected models and still keeps role data on the pivot.
🤖 Prompt for AI Agents
In app/Models/User.php lines 61 to 65, the workspaces() method currently returns
a HasMany relation to WorkspaceMembership models, which is misleading. Change
this method to return a BelongsToMany relation to Workspace models through the
workspace_memberships pivot table, including pivot data like roles. This will
provide actual Workspace models when calling $user->workspaces() while
preserving membership details on the pivot.
| <p className="text-sm">Online</p> | ||
| <p className="text-muted-foreground text-xs">{dayjs(status?.last_tested).fromNow()}</p> | ||
| </TooltipContent> |
There was a problem hiding this comment.
dayjs(...).fromNow() requires the relativeTime plugin
Without it, fromNow() is undefined and the component crashes.
-import dayjs from "dayjs";
+import dayjs from "dayjs";
+import relativeTime from "dayjs/plugin/relativeTime";
+
+dayjs.extend(relativeTime);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <p className="text-sm">Online</p> | |
| <p className="text-muted-foreground text-xs">{dayjs(status?.last_tested).fromNow()}</p> | |
| </TooltipContent> | |
| // at the top of resources/js/components/postgres/postgres-connection.tsx | |
| import dayjs from "dayjs"; | |
| import relativeTime from "dayjs/plugin/relativeTime"; | |
| dayjs.extend(relativeTime); | |
| // ...rest of your imports and component code |
🤖 Prompt for AI Agents
In resources/js/components/postgres/postgres-connection.tsx around lines 51 to
53, the usage of dayjs(...).fromNow() requires the relativeTime plugin to be
imported and extended. To fix this, import the relativeTime plugin from dayjs,
then extend dayjs with this plugin before using fromNow(), ensuring the
component does not crash due to undefined method.
| useEffect(() => { | ||
| const fetchStatus = async () => { | ||
| const response = await axios.get(route("postgres.connection-status", { id: connection.id })); | ||
| setStatus(response.data); | ||
| }; | ||
| fetchStatus(); | ||
| }, [connection.id]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
No error / unmount handling in fetchStatus
A failed request will throw an unhandled promise rejection and the effect keeps running on unmounted components.
- const fetchStatus = async () => {
- const response = await axios.get(route("postgres.connection-status", { id: connection.id }));
- setStatus(response.data);
- };
+ const controller = new AbortController();
+ const fetchStatus = async () => {
+ try {
+ const { data } = await axios.get(
+ route("postgres.connection-status", { id: connection.id }),
+ { signal: controller.signal }
+ );
+ setStatus(data);
+ } catch (e) {
+ if (!axios.isCancel(e)) console.error(e);
+ }
+ };
fetchStatus();
- }, [connection.id]);
+ return () => controller.abort();
+ }, [connection.id]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| const fetchStatus = async () => { | |
| const response = await axios.get(route("postgres.connection-status", { id: connection.id })); | |
| setStatus(response.data); | |
| }; | |
| fetchStatus(); | |
| }, [connection.id]); | |
| useEffect(() => { | |
| const controller = new AbortController(); | |
| const fetchStatus = async () => { | |
| try { | |
| const { data } = await axios.get( | |
| route("postgres.connection-status", { id: connection.id }), | |
| { signal: controller.signal } | |
| ); | |
| setStatus(data); | |
| } catch (e) { | |
| if (!axios.isCancel(e)) console.error(e); | |
| } | |
| }; | |
| fetchStatus(); | |
| return () => controller.abort(); | |
| }, [connection.id]); |
🤖 Prompt for AI Agents
In resources/js/components/postgres/postgres-connection.tsx around lines 15 to
21, the fetchStatus async function inside useEffect does not handle errors or
component unmounting, which can cause unhandled promise rejections and state
updates on unmounted components. To fix this, add a try-catch block around the
axios request to handle errors gracefully, and use a flag or AbortController to
check if the component is still mounted before calling setStatus, preventing
state updates after unmount.
| Route::get('/postgres', [PostgresConnectionController::class, 'index'])->name('postgres.index'); | ||
| Route::post('/postgres', [PostgresConnectionController::class, 'store'])->name('postgres.store'); | ||
| Route::get('/postgres/{connection:ulid}', [PostgresConnectionController::class, 'explore'])->name('postgres.explore'); | ||
| Route::get('/postgres/{connection}', [PostgresConnectionController::class, 'explore'])->name('postgres.explore'); | ||
| Route::post('/postgres/test-connection', [PostgresConnectionController::class, 'testConnection'])->name('postgres.test-connection'); | ||
| Route::get('/postgres/connection-status/{id}', [PostgresConnectionController::class, 'connectionStatus'])->name('postgres.connection-status'); | ||
|
|
There was a problem hiding this comment.
Route ordering causes /postgres/test-connection to hit the generic {connection} route
/postgres/{connection} precedes the more specific /postgres/test-connection and will capture the string test-connection.
Re-order specific routes above the catch-all:
-Route::get('/postgres/{connection}', ...);
-Route::post('/postgres/test-connection', ...);
+Route::post('/postgres/test-connection', ...);
+Route::get('/postgres/{connection}', ...);Do the same for connection-status if you observe similar clashes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Route::get('/postgres', [PostgresConnectionController::class, 'index'])->name('postgres.index'); | |
| Route::post('/postgres', [PostgresConnectionController::class, 'store'])->name('postgres.store'); | |
| Route::get('/postgres/{connection:ulid}', [PostgresConnectionController::class, 'explore'])->name('postgres.explore'); | |
| Route::get('/postgres/{connection}', [PostgresConnectionController::class, 'explore'])->name('postgres.explore'); | |
| Route::post('/postgres/test-connection', [PostgresConnectionController::class, 'testConnection'])->name('postgres.test-connection'); | |
| Route::get('/postgres/connection-status/{id}', [PostgresConnectionController::class, 'connectionStatus'])->name('postgres.connection-status'); | |
| Route::get('/postgres', [PostgresConnectionController::class, 'index'])->name('postgres.index'); | |
| Route::post('/postgres', [PostgresConnectionController::class, 'store'])->name('postgres.store'); | |
| - Route::get('/postgres/{connection}', [PostgresConnectionController::class, 'explore'])->name('postgres.explore'); | |
| - Route::post('/postgres/test-connection', [PostgresConnectionController::class, 'testConnection'])->name('postgres.test-connection'); | |
| + Route::post('/postgres/test-connection', [PostgresConnectionController::class, 'testConnection'])->name('postgres.test-connection'); | |
| + Route::get('/postgres/{connection}', [PostgresConnectionController::class, 'explore'])->name('postgres.explore'); | |
| Route::get('/postgres/connection-status/{id}', [PostgresConnectionController::class, 'connectionStatus'])->name('postgres.connection-status'); |
🤖 Prompt for AI Agents
In routes/web.php around lines 18 to 23, the route for '/postgres/{connection}'
is defined before the more specific routes '/postgres/test-connection' and
'/postgres/connection-status/{id}', causing the specific routes to be captured
by the generic '{connection}' route. To fix this, reorder the routes by placing
the more specific routes '/postgres/test-connection' and
'/postgres/connection-status/{id}' above the generic '/postgres/{connection}'
route to ensure correct route matching.
| $connections = DatabaseConnection::where('type', 'postgres') | ||
| ->where('user_id', $request->user()->id) | ||
| ->get(); |
There was a problem hiding this comment.
Add workspace filtering to the index query.
The query retrieves all postgres connections without filtering by workspace. This could expose connections from other workspaces to unauthorized users.
Apply this diff to add workspace filtering:
$connections = DatabaseConnection::where('type', 'postgres')
+ ->where('workspace_id', $request->user()->current_workspace_id)
->get();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $connections = DatabaseConnection::where('type', 'postgres') | |
| ->where('user_id', $request->user()->id) | |
| ->get(); | |
| $connections = DatabaseConnection::where('type', 'postgres') | |
| ->where('workspace_id', $request->user()->current_workspace_id) | |
| ->get(); |
🤖 Prompt for AI Agents
In app/Http/Controllers/Postgres/PostgresConnectionController.php around lines
49 to 50, the query fetching postgres connections lacks filtering by workspace,
risking exposure of connections across workspaces. Modify the query to include a
where condition that filters connections by the current workspace ID, ensuring
only connections belonging to the authorized workspace are retrieved.
| public function connectionStatus(Request $request, $id) | ||
| { | ||
| $connection = DatabaseConnection::where('id', $id)->first(); | ||
|
|
||
| return $connection; | ||
| }); | ||
| try { | ||
| $credentials = [ | ||
| 'host' => $connection->host, | ||
| 'port' => $connection->port, | ||
| 'username' => $connection->username, | ||
| 'password' => $connection->password, | ||
| 'database' => $connection->database, | ||
| ]; | ||
|
|
||
| return Inertia::render('postgres/postgres', [ | ||
| 'connections' => $connectionsWithStatus, | ||
| ]); | ||
| $isOnline = $this->connectionManager->testConnection($credentials); | ||
|
|
||
| return response()->json(['connection_status' => $isOnline, 'last_tested' => now(), 'connection_error' => null]); | ||
| } catch (\Exception $e) { | ||
| return response()->json(['connection_status' => false, 'last_tested' => now(), 'connection_error' => $e->getMessage()]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add authorization check before revealing connection status.
The method retrieves and tests a connection without verifying the user has access to it. This could allow unauthorized users to probe connection status.
Add authorization check after retrieving the connection:
$connection = DatabaseConnection::where('id', $id)->first();
+
+if (!$connection || $connection->workspace_id !== $request->user()->current_workspace_id) {
+ return response()->json(['error' => 'Connection not found'], 404);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function connectionStatus(Request $request, $id) | |
| { | |
| $connection = DatabaseConnection::where('id', $id)->first(); | |
| return $connection; | |
| }); | |
| try { | |
| $credentials = [ | |
| 'host' => $connection->host, | |
| 'port' => $connection->port, | |
| 'username' => $connection->username, | |
| 'password' => $connection->password, | |
| 'database' => $connection->database, | |
| ]; | |
| return Inertia::render('postgres/postgres', [ | |
| 'connections' => $connectionsWithStatus, | |
| ]); | |
| $isOnline = $this->connectionManager->testConnection($credentials); | |
| return response()->json(['connection_status' => $isOnline, 'last_tested' => now(), 'connection_error' => null]); | |
| } catch (\Exception $e) { | |
| return response()->json(['connection_status' => false, 'last_tested' => now(), 'connection_error' => $e->getMessage()]); | |
| } | |
| } | |
| public function connectionStatus(Request $request, $id) | |
| { | |
| $connection = DatabaseConnection::where('id', $id)->first(); | |
| if (!$connection || $connection->workspace_id !== $request->user()->current_workspace_id) { | |
| return response()->json(['error' => 'Connection not found'], 404); | |
| } | |
| try { | |
| $credentials = [ | |
| 'host' => $connection->host, | |
| 'port' => $connection->port, | |
| 'username' => $connection->username, | |
| 'password' => $connection->password, | |
| 'database' => $connection->database, | |
| ]; | |
| $isOnline = $this->connectionManager->testConnection($credentials); | |
| return response()->json([ | |
| 'connection_status' => $isOnline, | |
| 'last_tested' => now(), | |
| 'connection_error' => null, | |
| ]); | |
| } catch (\Exception $e) { | |
| return response()->json([ | |
| 'connection_status' => false, | |
| 'last_tested' => now(), | |
| 'connection_error' => $e->getMessage(), | |
| ]); | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/Http/Controllers/Postgres/PostgresConnectionController.php around lines
62 to 81, the connectionStatus method lacks an authorization check before
returning connection status, which risks exposing sensitive information. After
retrieving the connection by ID, add an authorization check to verify the
current user has permission to access this connection, using Laravel's
authorization features like $this->authorize or a policy. If unauthorized,
return an appropriate error response to prevent unauthorized access.
- Updated workspace ID assignment in the HasWorkspaceScope trait to use null coalescing assignment. - Enhanced error message formatting in PostgresQueryController for better readability. - Refactored workspace data sharing in HandleInertiaRequests middleware to streamline user workspace information. - Improved workspace membership verification in WorkspaceMiddleware for better clarity. - Wrapped workspace and membership creation in a transaction in RegisteredEvent to ensure data integrity. - Changed the query column type in the database_queries migration from string to longText for better handling of complex queries. - Updated foreign key constraints in user sessions and workspace migrations for improved data integrity.
- Added an index on the 'created_at' column in the database_queries migration for improved query performance. - Updated the workspace selector component to use 'switch_workspace' instead of 'switch_team' for better clarity in data handling.
1 participant
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Chores
Refactor
Documentation
Style
Tests
.gitignorefor debugbar storage to maintain a clean development environment.