Skip to content

Review Dashboard Page - #383

Open
dan-tang-ssd wants to merge 17 commits into
fix-application-testsfrom
review-dashboard-page-3
Open

Review Dashboard Page#383
dan-tang-ssd wants to merge 17 commits into
fix-application-testsfrom
review-dashboard-page-3

Conversation

@dan-tang-ssd

@dan-tang-ssd dan-tang-ssd commented Aug 17, 2026

Copy link
Copy Markdown
Member

This PR is submitted to fix #375

It is not yet ready for review. It is submitted for progress update.

It contains below changes:

  • install Laravel boost package, add config files
  • fix 5 security issues (see below comments for details)
  • fix 5 performance issues (see below comments for details)

@dan-tang-ssd

Copy link
Copy Markdown
Member Author

Fixed security issues 1 to 5 as described below:

  1. Any authenticated user can pull any other institution's full, non-anonymized data. Make sure the logged in user has access rights to the organisation that is being accessed.
  2. Two-layer SQL injection, not just an unvalidated ID. Add a Laravel form request to validate IDs in form submission data. Also prevent SQL injection for any SQL harmful for the database.
  3. The one field that is guarded isn't the only one that needed guarding. We have a checking to make sure there are more than 3 instituions and 10 projects for principle summary result. We should apply the same checking to red flag summary result.
  4. Switching organisations is existence-checked, not membership-checked. This is to make sure the selected organisation is accessible by the logged in user.
  5. Debug instrumentation shipped in the live path. Remove ray() statements and remove variable that is never used.

…n a WHERE clause. Change WHERE clause condition from function-based to set-based. This reduces number of queries to be executed
…s can collide; Generate uncollidable dashboard ID; Add command to schedule job to remove dashboard related records old than 1 hour
…t request after 400ms once fully entered filter budget value, avoid submitting request for every keystroke; narrow to only touch the rows this specific comparison actually needs, regardless of total system size
…based aggregation; the redlinesSummary cursor/FETCH loop + string CONCAT is replaced with a single JSON_ARRAYAGG(JSON_OBJECT(...)) query. Removed the now-dead cursor, handler, and loop-only variable declarations
@dan-tang-ssd

Copy link
Copy Markdown
Member Author

Fixed performance issues 1 to 5 as described below:

  1. Every hot join/filter column in the flow is unindexed

    • Add indexes to database columns in join queries.
  2. A scalar function evaluated once per row, in a WHERE clause.

    • fix the previously found performance issue:
      • This section takes the runtime from ~150ms to ~2.5s. Is there a way of optimising this?
    • reduce number of queries by changing a WHERE clause condition from function-based (3 more queries per candidate row) to set-based
  3. Scratch tables are never cleaned up, and IDs can collide.

    • Generate uncollidable dashboard ID
    • Add command to schedule job to remove dashboard related records older than 1 hour
  4. Every keystroke is a full round trip.

    • submit request after 400ms once fully entered filter budget value, avoid submitting request for every keystroke
    • narrow to only touch the rows this specific comparison actually needs, regardless of total system size
  5. Row-at-a-time JSON building instead of set-based aggregation.

    • the redlinesSummary cursor/FETCH loop + string CONCAT is replaced with a single JSON_ARRAYAGG(JSON_OBJECT(...)) query. Removed the now-dead cursor, handler, and loop-only variable declarations

…d be treated as string with leading zero instead of integer; add try catch block to avoid spinner get stuck; show alert if error occurred
…ives or institutions within the current set of filters
@dan-tang-ssd

dan-tang-ssd commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

I have fixed 5 security issues and 5 performance issues in this PR.
Here is the BEFORE and AFTER artifact for reference.

BEFORE artifact:
https://claude.ai/code/artifact/2331fb6a-1f6b-4e57-bc1f-7ad69ff562dc?via=auto_preview

AFTER artifact:
https://claude.ai/code/artifact/0bb2eabf-0e29-4ae5-aa0c-ae48f78c3519?via=auto_preview


I have imported live db mysqldump file into local db for testing and comparison.
I opened two browsers, one for live env and the other for local env.
I selected the same institution in both env, then go to dashboard page.
Local env shows response much faster than live env.

I tried 3 different institutions:

Live env: 11.99 s    Local env: 3.76 s
Live env: 15.92 s    Local env: 5.25 s
Live env: 13.27 s    Local env: 4.24 s

There are two remaining items for revising dashboard page:

  1. Pre-calculate assessment result for all initiatives, use it for OTHERS
  2. Allow comparison in a more flexible way. e.g. Portfolio A to Portfolio B; Portfolio A to All my portfolios, etc

I will work on them in separate PR.

@dan-tang-ssd

Copy link
Copy Markdown
Member Author

Note for deployment procedure:

  1. Currently dashboard_xxx.created_at is NULL. In order to let the prune dashboard command works properly. We will need to set dashboard_xxx.created_at as NOW() in live database.

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.

1 participant